wtclient: handle un-acked updates for exhausted sessions #8233
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
ellemouton
force-pushed
the
unackedUpdatesBug
branch
2 times, most recently
from
5b40fdb to
376a7cf
Compare
ellemouton
force-pushed
the
unackedUpdatesBug
branch
from
376a7cf to
844a0c1
Compare
bhandras
approved these changes
Jan 8, 2024
ellemouton
force-pushed
the
unackedUpdatesBug
branch
from
844a0c1 to
cc224a1
Compare
bitromortac
reviewed
Jan 11, 2024
This commit adds a test to demonstrate an edge case that can result in the "tower has un-acked updates" error being thrown when a user is attempting to remove a tower. This will be fixed in an upcoming commit.
ellemouton
force-pushed
the
unackedUpdatesBug
branch
from
cc224a1 to
64d319a
Compare
|
@bhandras - gonna re-request since i've updated as per @bitromortac's comment about not needing the boolean option 🙏 |
bhandras
approved these changes
Jan 11, 2024
In this commit, we adjust the PostEvaluateFilterFn to also take in a count representing the number of committed updates (ie, persisted un-acked updates) that the session has. This will be made use of in an upcoming commit.
In this commit, we use the newly added session listing options to ensure that we only see a session as exhausted if it does not have any un-acked updates on disk. This fixes the bug previously demonstrated.
ellemouton
force-pushed
the
unackedUpdatesBug
branch
from
64d319a to
b14b30f
Compare
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.
This PR addresses an edge case that could lead to the "tower has un-acked updates" error
when a user tries to remove a tower. See the issue here.
The Issue:
Currently, if you call "remove tower", then only sessions from that tower that are loaded into memory on
startup are terminated correctly. And we only load in sessions where
session.SeqNum < session.policy.MaxUpdates.So the issue occurs when the session is technically exhausted but there is still an un-acked update on disk.
This means that we would not terminate such a session properly on tower removal and so would run into "tower has un-acked updates".
The Solution:
So with this PR, we make sure to load a session into memory if there are still un-acked updates that it needs to deal with.
Flow of the PR: