-
Notifications
You must be signed in to change notification settings - Fork 15
REP-5201 Persist the change stream’s resume token. #30
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
Merged
Merged
Changes from all commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
3659c0a
persist
FGasper c4a9b72
old method
FGasper 07e2691
switch to always using resume token’s timestamp
FGasper a13cea7
fix StartChangeStream
FGasper 0044884
Merge branch 'main' into REP-5201-persist-resume-token
FGasper 2ed30e1
tweak timestamp extraction
FGasper 7d900c0
save
FGasper 3b18845
fix
FGasper 8a39982
Merge branch 'main' into REP-5201-persist-resume-token
FGasper df8c59d
Merge branch 'main' into REP-5201-persist-resume-token
FGasper e65dd09
bump Go requirement to 1.20
FGasper 346cbde
string
FGasper 3490a04
fix test
FGasper e5ed6ba
reverse error check
FGasper a218e84
fix most lint issues
FGasper 8ffc161
fix outstanding
FGasper c32edc8
update Go requirement
FGasper 6851b57
update CI Go version
FGasper 3017954
Merge branch 'fix_lint' into REP-5201-persist-resume-token
FGasper 6833b7c
add resumability
FGasper 22cbc00
fix test
FGasper 917b5c0
tests against change stream resumption
FGasper bc76160
Merge branch 'main' into fix_lint
FGasper b321bd0
Merge branch 'fix_lint' into REP-5201-persist-resume-token
FGasper 3f37acd
restore test
FGasper d1e3bb8
fix ordering assertion
FGasper 6fe3ba6
fix error
FGasper 67230b6
update README
FGasper 1d3af02
tweak wording
FGasper b368006
Update internal/verifier/change_stream.go
FGasper 5a54633
Evgeni’s review.
FGasper 36a6cac
strike paragraph
FGasper 3b26420
refactor & solve inconsistency
FGasper 2b0a3f8
remove dupe wrap
FGasper 57a155a
error-check the resume token persistence
FGasper 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 hidden or 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 file contains hidden or 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.
I'd be wary of printing out the whole resume token as-is. This caused a fatal error in REP-3625, so I think we should either truncate it or omit it. Thoughts?
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 would think that not to happen here because the verifier will run simpler change streams than mongosync does under ORR, but yeah it’s probably best not to chance it.
Replacing with an extracted timestamp.
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 REP-3625 is evergreen unable to parse a long log line, which seems to have been fixed by evergreen.
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.
Even so, it seems best to err here on the side of caution. We probably don’t need the full resume token in the log.