Choose sync peer other than leader when possible #2041
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danieldoglas
requested review from
cead22,
iwiznia and
tylerkaraszewski
and removed request for
cead22 and
iwiznia
iwiznia
previously approved these changes
Dec 24, 2024
| peer->setCommit(message.calcU64("CommitCount"), message["Hash"]); | ||
|
|
||
|
|
||
| // We check the commit difference with 12,500 commits behind because that |
Contributor
There was a problem hiding this comment.
Is this change supposed to be in this PR? Seems kind of unrelated to the other change.
Contributor
There was a problem hiding this comment.
I think this PR should be on hold til the other one is merged
Contributor
Author
There was a problem hiding this comment.
this was the other one: #2039
I changed the base branch to that PR so you can see only the necessary changes.
Contributor
|
Change looks good. Happy to approve when the other PR is merged and this one only has the diff for this change |
danieldoglas
changed the base branch from
main
to
dsilva_blockingCommandPortWhenNotCaughtUp
Contributor
Author
|
Changed the base PR so it only shows the needed changes |
…chooseSyncPeerOtherThanLeader
Base automatically changed from
dsilva_blockingCommandPortWhenNotCaughtUp
to
main
December 26, 2024 18:48
tylerkaraszewski
dismissed
iwiznia’s stale review
The base branch was changed.
Contributor
Author
iwiznia
previously approved these changes
Dec 27, 2024
Contributor
|
Approved (though not showing for me in the right pane), anyway leaving it for @cead22 and @tylerkaraszewski to review too |
cead22
previously approved these changes
Dec 30, 2024
tylerkaraszewski
requested changes
Dec 30, 2024
cead22
approved these changes
Dec 31, 2024
tylerkaraszewski
approved these changes
Dec 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
This PR changes the logic in
_updateSyncPeerto choose the leader as a sync peer only if no other nodes are available. The change should help reduce processing pressure on the leader node.Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/455682
Tests
Internal Testing Reminder: when changing bedrock, please compile auth against your new changes