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
Don't update remote client CIDs gratuitously #1294
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with this logic, so this is a bit of a superficial review.
@Matthias247 would you mind reviewing this?
Handling retire_prior_to might have already moved us off the initial CID.
0facb62
to
cc494dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Still going to take a more thorough look at the changes here.)
That looks like an impressive refactoring. My confidence in my own judgement is still low, but it looks good to me. (And especially with the added documentation and test coverage it feels like a clear improvement.) |
This predicate was based on whether an entry in the circular CID buffer was unset. However, that entry might have already been overwritten with newer data. Meanwhile, because we guarantee that the active CID is always at the start of the sliding window of CID sequence numbers, we can easily infer whether the previously-active CID was retired and replaced by retire_prior_to by whether the retired set it returns is empty.
We were disabling it every time we updated the remote CID, and we like to update the remote CID ASAP for servers to enable client-side stateless resets. Instead, we should disable it only when switching remote CIDs due to a migration.
Tidied things up a bit more, in particular to get rid of the dubious |
Duplicates of the in-use remote CID do not need to be handled differently from duplicates of buffered, unused CIDs.
We should probably timeout on additional reviews and just get this merged. It seems unlikely to be worse given the added tests! |
Handling retire_prior_to might have already moved us off the initial CID.