Skip to content

Simplify syncer code.#879

Open
ggreer wants to merge 1 commit into
mainfrom
ggreer/syncer-cleanup
Open

Simplify syncer code.#879
ggreer wants to merge 1 commit into
mainfrom
ggreer/syncer-cleanup

Conversation

@ggreer
Copy link
Copy Markdown
Contributor

@ggreer ggreer commented May 26, 2026

Use the parallel syncer for all syncs. Default to 1 worker so behavior is the same.

@ggreer ggreer requested a review from a team May 26, 2026 22:21
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026 •
edited
Loading

General PR Review: Simplify syncer code.

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Review mode: full
View review run

Review Summary

This PR removes the sequentialSync method (~220 lines), the noOpMutex/rwMutex abstraction in progresslog, and the WithSequentialMode option, unifying all syncs through parallelSync with a default of 1 worker for equivalent sequential behavior. The previous review's suggestion about the dead if s.workerCount > 0 condition has been addressed—the condition and the WithSequentialMode option it guarded were both removed entirely. No new issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

github-actions[bot]
github-actions Bot reviewed May 26, 2026
View reviewed changes
Comment thread pkg/sync/syncer.go
return err
}

// Force a checkpoint to clear completed actions & entitlement graph in sync_token.
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: This condition s.workerCount > 0 is now always true since workerCount is guaranteed >= 1 after this PR. Consider changing to s.workerCount > 1 to preserve sequential-mode progress reporting for single-worker syncs, or remove the condition entirely.

github-actions[bot]
github-actions Bot reviewed May 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@ggreer
Simplify syncer code.
093074b 
Use the parallel syncer for all syncs. Default to 1 worker so behavior is the same.
@ggreer ggreer force-pushed the ggreer/syncer-cleanup branch from 3568166 to 093074b Compare May 26, 2026 22:27
github-actions[bot]
github-actions Bot reviewed May 26, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@ggreer