-
Notifications
You must be signed in to change notification settings - Fork 15
REP-6804 Simplify change handling logic #173
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
Conversation
| select { | ||
| case <-e.ready: | ||
| return e.val | ||
| default: | ||
| panic("Eventual's Get() called before value was ready.") |
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 seems that Eventual can only be read once per write. The comment says it's like context.Context’s Done() and Err(), is that changed with this PR?
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 added a test that confirms that a 2nd time works.
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’s not changed … I added a test that confirms that a 2nd read works.
tdq45gj
left a comment
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.
LGTM. Thanks!
Previously the code maintained:
This is overly complex.
This changeset simplifies the above. Now there is a single Eventual that holds the final status of both readers & both persistors. Together the reader & persistor threads are called “change handling”.
This also fixes a small bug in Eventual: previously it was impossible to store a nil value because Eventual was implemented via Option. Now it’s possible to store nil.