Time out SUBSCRIBE query instead of forking. #1783
Merged
Conversation
This file contains 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
tylerkaraszewski
changed the title
cead22
reviewed
Jun 21, 2024
sqlitecluster/SQLiteNode.cpp
Outdated
|
|
||
| // We set this for all commits because this only gets all commits in response to SUBSCRIBE, which is done synchronously, and blocks the commit thread. | ||
| // For asynchronous queries, there's nothing being blocked, so it doesn't much matter how long these take. | ||
| // This is really not the correct encapsulation for this, but we can improve that later. |
There was a problem hiding this comment.
Pet peeve, but this is a TODO in a comment. Can we make a GH issue for it instead?
Suggested change
| // This is really not the correct encapsulation for this, but we can improve that later. |
sqlitecluster/SQLiteNode.cpp
Outdated
| // We set this for all commits because this only gets all commits in response to SUBSCRIBE, which is done synchronously, and blocks the commit thread. | ||
| // For asynchronous queries, there's nothing being blocked, so it doesn't much matter how long these take. | ||
| // This is really not the correct encapsulation for this, but we can improve that later. | ||
| timeoutLimitUS = 10'000'000; |
There was a problem hiding this comment.
Since the cluster gives up on the leader node after 30s, should we make this higher and closer to 30s, say 25?
If there's a reason for 10 and it's not an arbitrary number, let's add a comment
iwiznia
reviewed
Jun 24, 2024
sqlitecluster/SQLite.cpp
Outdated
| _timeoutLimit = STimeNow() + timeoutLimitUS; | ||
| } | ||
| int queryResult = SQuery(_db, "getting commits", query, result); | ||
| _timeoutLimit = 0; |
There was a problem hiding this comment.
Should we be calling clearTimeout here?
iwiznia
changed the title
iwiznia
approved these changes
Jun 24, 2024
|
Updated testing on the latest change showing the same behavior as the original testing: |
tylerkaraszewski
changed the title
cead22
approved these changes
Jun 24, 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
See issue, it's pretty detailed.
Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/406697
Tests
Testing on commit e146093 (before rolling back the test hacks):
We successfully time out the query:
This kills the peer connection:
Follower reconnects, switches to searching:
Follower runs through the state machine, succeeds on second try:
Auth built and tested against this as well.
Internal Testing Reminder: when changing bedrock, please compile auth against your new changes