-
Notifications
You must be signed in to change notification settings - Fork 532
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
Store handles in detached DDS before attaching #21132
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
22aaeb9
Add tests for storing handles detached, then storing in an attached d…
jzaffiro 9cdec05
abstraction for map
jzaffiro babed31
WIP: test clean up
jzaffiro 7330bdb
Merge branch 'main' into moreAttachTests
jzaffiro 66a6c13
fix import
jzaffiro 0455b0d
Add set of tests that use a new data store and keep those that don't …
jzaffiro 71a4dcd
Ensure all handles are bound on dds attach
anthony-murphy 010315f
Merge remote-tracking branch 'anthony-murphy/fix-handle-binding' into…
jzaffiro b06a239
Remove consensus queue tests and make storeHandle async
jzaffiro File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
Oops, something went wrong.
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.
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.
@agarwal-navin: @anthony-murphy and I were wondering if this is an ok way to solve the problem of contexts for handles stored in other detached DDSs not being seen. The goal is to ensure that all contexts, even those for handles stored in a detached DDS, are able to be resolved and the handles will still get bound.
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.
this solve the problem where binding a context results in another context binding, which the current code can miss, if the content that gets bound is before the context that binds it in the contexts list
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.
it would be good to get this in and start back porting to rc3 and rc4. we may find a better way to do this, but i think we can forward fix in main if we need to
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.
@jzaffiro Can you explain the bug to me that you are trying to fix? The description says this is adding tests but it has code changes as well so I am not clear what is being solved here :-).
Is the problem only what Tony pointed out - "if the content that gets bound is before the context that binds it in the contexts list"? Basically, due to ordering we can miss some contexts?
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.
The tests are to make sure we don't hit a bug where handles that are stored in detached DDSs are not bound when we attach. The tests starting on line 501 of handleValidation.spec.ts were not able to find the DDS referenced to in the other detached DDS, which is what this code here solves. I'll update the description as well.
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.
Before #20998, we were also seeing a bug where going from detached --> attached would cause duplicate attach ops to be sent, and the tests starting on line 405 make sure we have coverage for all DDSs for that bug