-
Notifications
You must be signed in to change notification settings - Fork 0
Make View maintain ViewData references, bug fixes #8
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
batch_value = self._batch_map.get(key) | ||
if batch_value: | ||
# Update the stored checkpointed value if exists. The call to merge | ||
# here combines only identical records (same key). | ||
batch_value.merge(aggregator) |
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 think this case was for the race condition when multiple process
occurs at the same time. I don't think you can just call take_checkpoint()
because the timestamps would not be correct.
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.
From what I understand the reason this existed is because if you have multiple boundinstruments which both own ViewDatas with the same key, we need to merge the ViewDatas. However now since there is guaranteed to be only 1 viewdata per key we don't need to merge them anymore
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 think this logic existed even before views existed, so it must have been for something else.
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.
not sure then. if I add an assert statement assert batch_value == aggregator
, all tests will pass, which makes me think that atleast at our testing amount this has no other effect (edit: let me actually verify this.. some tests are failing for other reasons).
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.
fixed
No description provided.