Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Choose sync peer other than leader when possible #2041

Open
wants to merge 2 commits into
base: dsilva_blockingCommandPortWhenNotCaughtUp
Choose a base branch
from

Conversation

danieldoglas
Copy link
Contributor

Details

This PR changes the logic in _updateSyncPeer to 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

@danieldoglas danieldoglas self-assigned this Dec 24, 2024
@danieldoglas danieldoglas requested review from iwiznia, cead22 and tylerkaraszewski and removed request for iwiznia and cead22 December 24, 2024 15:48
@@ -1278,7 +1278,19 @@ void SQLiteNode::_onMESSAGE(SQLitePeer* peer, const SData& message) {
peer->commandAddress = message["commandAddress"];
}
peer->setCommit(message.calcU64("CommitCount"), message["Hash"]);


// We check the commit difference with 12,500 commits behind because that
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change supposed to be in this PR? Seems kind of unrelated to the other change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR should be on hold til the other one is merged

Copy link
Contributor

Choose a reason for hiding this comment

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

What is the "other one"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the other one: #2039

I changed the base branch to that PR so you can see only the necessary changes.

@cead22
Copy link
Contributor

cead22 commented Dec 24, 2024

Change looks good. Happy to approve when the other PR is merged and this one only has the diff for this change

@danieldoglas danieldoglas changed the base branch from main to dsilva_blockingCommandPortWhenNotCaughtUp December 24, 2024 19:03
@danieldoglas
Copy link
Contributor Author

danieldoglas commented Dec 24, 2024

Changed the base PR so it only shows the needed changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants