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

Bootstrap server binding read-message in a single read_exact #3774

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

Ben-PH
Copy link
Contributor

@Ben-PH Ben-PH commented Apr 5, 2023

This allows the bootstrap server to perform a complete read in one go, without incremental consumption of the stream (...internally, read_exact is an incremental read, but sorting that out is another issue. This statement assumes read_exact functions non-incrementally)

This allows state-changes in the server to be complete: The server consumes a message completely, or not at all.

  • document all added functions
  • try in sandbox /simulation/labnet
  • unit tests on the added/changed features
    • make tests compile
    • make tests pass
  • add logs allowing easy debugging in case the changes caused problems
  • if the API has changed, update the API specification

@Ben-PH Ben-PH marked this pull request as ready for review April 5, 2023 17:09
@Ben-PH Ben-PH linked an issue Apr 5, 2023 that may be closed by this pull request
25 tasks
@Ben-PH Ben-PH mentioned this pull request Apr 5, 2023
25 tasks
@Ben-PH Ben-PH self-assigned this Apr 6, 2023
@Ben-PH Ben-PH added the bootstrap Issues related to the bootstrap label Apr 6, 2023
@Ben-PH Ben-PH requested a review from AurelienFT April 7, 2023 17:15
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

I don't really understand why you changed one read_exact with a peek and not the others to read bigger after. Even with the comment this change is unclear to me

EDIT: Ok I see, it's to say atomic on the read

@Ben-PH
Copy link
Contributor Author

Ben-PH commented Apr 12, 2023

EDIT: Ok I see, it's to say atomic on the read

That's the idea. If for whatever reason, the attempt to get the next message fails halfway through (in the case of this change, after the stream is peeked, but before the read_exact occurs), the stream is unchanged.

In an ideal world, the state of the data in the stream only changes if the state of the server changes, and that state change is a before/after. No "during".

@Ben-PH Ben-PH merged commit 56122d1 into bootstrap/mocked-and-sync Apr 12, 2023
@Ben-PH Ben-PH mentioned this pull request Apr 28, 2023
3 tasks
@AurelienFT AurelienFT deleted the bootstrap_server/next-single-read branch May 29, 2023 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bootstrap Issues related to the bootstrap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bootstrap improvements tracking issue
2 participants