This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Update events stream cache before ID generator #14648
Closed
Conversation
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
This means the entire lifecycle of both the caches and ID generators for events are handled by the one class.
This means during replication the changes will be flagged twice, but this isn't a problem aside from pointless work. To avoid this requires bringing the function out of the `CacheInvalidationWorkerStore` which is much more involved due to class dependencies.
This will assert that any changes being flagged have tokens ahead of the current known position. If the change is at or behind the current token there is a race condition during which changes may be incorrectly missed, this guards against that.
Fizzadar
force-pushed
the
update-events-stream-cache-before-id
branch
from
5ee0425
to
3a86f75
Compare
This has to be here to account for applying changes on workers that acually increment the stream ID - they have to increment the token before they know about it to flag the change. This does leave open a potential hole in the check for replicated changes which should be addressed in the future.
Fizzadar
changed the title
Fizzadar
commented
Dec 10, 2022
| @@ -271,6 +277,13 @@ def entity_has_changed(self, entity: EntityType, stream_pos: int) -> None: | |||
| if stream_pos <= self._earliest_known_stream_pos: | |||
| return | |||
|
|
|||
| # Any change being flagged must be ahead of any current token, otherwise | |||
| # we have a race condition between token position and stream change cache. | |||
| # NOTE: this checks for equal to allow for a process persisting an event to | |||
There was a problem hiding this comment.
Not super pleased with this perhaps a better way would be to add a flag to explicitly enable exact matches for writer instances?
|
I have reverted to draft because #14723 presents a cleaner (IMO) solution (but not the assertion - perhaps worth splitting into a separate PR?). |
|
looks like this is largely superceded by #14723? The assertion seems like a worthwhile addition, via a separate PR. |
Yes and agreed, will make a separate PR for that when I have a moment, closing this one! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Part of #14158
Essentially this just moves all the handling logic for the events stream change cache and ID generator under the
EventsWorkerStoreand ensures that the cache is updated before the token position. This prevents other threads seeing tokens ahead of the cache (full description at #14158 (comment)).This now also includes an assertion in flagging changes in the stream change cache, as described in this comment: #14158 (comment).
Signed off by Nick @ Beeper (@Fizzadar).
Pull Request Checklist
(run the linters)