-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Engine: version logic on replicas should not be hard coded #23998
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
The refactoring in elastic#23711 hardcoded version logic for replica to assume monotonic versions. Sadly that's wrong for `FORCE` and `VERSION_GTE`. Instead we should use the methods in VersionType to detect conflicts. Note - once replicas use sequence numbers for out of order delivery, this logic goes away.
| version = i; | ||
| break; | ||
| case EXTERNAL_GTE: | ||
| version = randomBoolean() && i > 0 ? i - 1 : i; |
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.
Is this clearer: version = randomBoolean() ? Math.max(i - 1, 0) : i;?
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.
sure.
jasontedor
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. I left one comment. Is the title of the pull request/commit message correct?
No... it missed a |
The refactoring in #23711 hardcoded version logic for replica to assume monotonic versions. Sadly that's wrong for `FORCE` and `VERSION_GTE`. Instead we should use the methods in VersionType to detect conflicts.
The refactoring in #23711 hardcoded version logic for replica to assume monotonic versions. Sadly that's wrong for
FORCEandVERSION_GTE. Instead we should use the methods in VersionType to detect conflicts.Note - once replicas use sequence numbers for out of order delivery, this logic goes away.