Improve lost-increment message in repo analysis #131200
Merged
Conversation
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
Today repository analysis may fail with a message like the following:
[test-repo] register [test-register-contended-F_NNXHrSSDGveoeyj1skwg]
should have value [10] but instead had value
[OptionalBytesReference[00 00 00 00 00 00 00 09]]
This is confusing because one might interpret `should have value [10]`
as an indication that Elasticsearch definitely wrote this value to the
register, leaving you trying to work out how that particular write was
lost. In fact it can be more subtle than that, we only believe the
register blob should have this value because we know we completed 10
supposedly-atomic increment operations, and the failure could instead be
that these operations are not as atomic as they need to be and that one
or more of the increments was lost.
This commit makes the message more verbose, clarifying that this failure
could be an atomicity problem rather than a simple lost write:
[test-repo] successfully completed all [10] atomic increments of
register [test-register-contended-F_NNXHrSSDGveoeyj1skwg] so its
expected value is [OptionalBytesReference[00 00 00 00 00 00 00 0a]],
but reading its value with [getRegister] unexpectedly yielded
[OptionalBytesReference[00 00 00 00 00 00 00 09]]
DaveCTurner
added
>enhancement
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
ywangd
reviewed
Jul 15, 2025
| value is [%s], but reading its value with [%s] unexpectedly yielded [%s]. This \ | ||
| anomaly may indicate an atomicity failure amongst concurrent compare-and-exchange \ | ||
| operations on registers in this repository.""", | ||
| expectedFinalRegisterValue, |
There was a problem hiding this comment.
This is not the number of increments, right? Do you mean to pass it down from the caller of finalRegisterValueVerifier?
There was a problem hiding this comment.
It is the number of increments, yes: it counts the number of successful responses to a ContendedRegisterAnalyzeAction (each of which does one increment operation). The caller of finalRegisterValueVerifier doesn't know this yet, because this is called before any of the actions have run.
We could simplify this because we always either increment this value or fail the whole analysis (skipping these checks). IIRC that wasn't always the case in some earlier draft and this never got cleaned up.
There was a problem hiding this comment.
OK I see the content is doubled as the count as well since it starts from 0 and increment 1 each time. Thanks for explaining.
There was a problem hiding this comment.
I opened #131274
mridula-s109
pushed a commit
to mridula-s109/elasticsearch
that referenced
this pull request
Jul 17, 2025
Today repository analysis may fail with a message like the following:
[test-repo] register [test-register-contended-F_NNXHrSSDGveoeyj1skwg]
should have value [10] but instead had value
[OptionalBytesReference[00 00 00 00 00 00 00 09]]
This is confusing because one might interpret `should have value [10]`
as an indication that Elasticsearch definitely wrote this value to the
register, leaving you trying to work out how that particular write was
lost. In fact it can be more subtle than that, we only believe the
register blob should have this value because we know we completed 10
supposedly-atomic increment operations, and the failure could instead be
that these operations are not as atomic as they need to be and that one
or more of the increments was lost.
This commit makes the message more verbose, clarifying that this failure
could be an atomicity problem rather than a simple lost write:
[test-repo] Successfully completed all [10] atomic increments of
register [test-register-contended-F_NNXHrSSDGveoeyj1skwg] so its
expected value is [OptionalBytesReference[00 00 00 00 00 00 00 0a]],
but reading its value with [getRegister] unexpectedly yielded
[OptionalBytesReference[00 00 00 00 00 00 00 09]]. This anomaly may
indicate an atomicity failure amongst concurrent
compare-and-exchange operations on registers in this repository.
mridula-s109
pushed a commit
to mridula-s109/elasticsearch
that referenced
this pull request
Jul 17, 2025
Today repository analysis may fail with a message like the following:
[test-repo] register [test-register-contended-F_NNXHrSSDGveoeyj1skwg]
should have value [10] but instead had value
[OptionalBytesReference[00 00 00 00 00 00 00 09]]
This is confusing because one might interpret `should have value [10]`
as an indication that Elasticsearch definitely wrote this value to the
register, leaving you trying to work out how that particular write was
lost. In fact it can be more subtle than that, we only believe the
register blob should have this value because we know we completed 10
supposedly-atomic increment operations, and the failure could instead be
that these operations are not as atomic as they need to be and that one
or more of the increments was lost.
This commit makes the message more verbose, clarifying that this failure
could be an atomicity problem rather than a simple lost write:
[test-repo] Successfully completed all [10] atomic increments of
register [test-register-contended-F_NNXHrSSDGveoeyj1skwg] so its
expected value is [OptionalBytesReference[00 00 00 00 00 00 00 0a]],
but reading its value with [getRegister] unexpectedly yielded
[OptionalBytesReference[00 00 00 00 00 00 00 09]]. This anomaly may
indicate an atomicity failure amongst concurrent
compare-and-exchange operations on registers in this repository.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Today repository analysis may fail with a message like the following:
This is confusing because one might interpret
should have value [10]as an indication that Elasticsearch definitely wrote this value to the
register, leaving you trying to work out how that particular write was
lost. In fact it can be more subtle than that, we only believe the
register blob should have this value because we know we completed 10
supposedly-atomic increment operations, and the failure could instead be
that these operations are not as atomic as they need to be and that one
or more of the increments was lost.
This commit makes the message more verbose, clarifying that this failure
could be an atomicity problem rather than a simple lost write: