Introduce constants for special ReservedClusterStateMetadata versions #107995
Conversation
mosche
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| @@ -50,7 +51,7 @@ ActionListener<ActionResponse.Empty> listener() { | |||
| static boolean isNewError(ReservedStateMetadata existingMetadata, Long newStateVersion) { | |||
| return (existingMetadata == null | |||
| || existingMetadata.errorMetadata() == null | |||
| || newStateVersion <= 0 // version will be -1 when we can't even parse the file, it might be 0 on snapshot restore | |||
| || newStateVersion <= RESTORED_VERSION // Long.MIN_VALUE (no version) when we can't parse the file, 0 on snapshot restore | |||
There was a problem hiding this comment.
I wonder, if there are no other cases, if
newStateVersion == RESTORED_VERSION || newStateVersion == EMPTY_VERSION is more clear (you could get rid of the comment, which is usually a good indication)
There was a problem hiding this comment.
I thought about this as well, and agree it would add clarity. At the same time I'm worried this would easily break things if more magic version ids are introduced. I'll think a bit more about it....
There was a problem hiding this comment.
Can we enforce the version for error metadata is > 0 when explicitly initialized (ie with a real error)?
There was a problem hiding this comment.
@rjernst I had another look at this, I think it's tricky to make any changes on error metadata version as it's coupled to the reserved state version. If a previous reserved state version exists, that one will be the version of the error. However, that can be any of the special versions <= 0.
There was a problem hiding this comment.
@ldematte changed as suggested
| return true; | ||
| } | ||
|
|
||
| // Version 0 is special, snapshot restores will reset to 0. | ||
| if (reservedStateVersion.version() <= 0L) { | ||
| if (reservedStateVersion.version() <= ReservedStateMetadata.RESTORED_VERSION) { |
There was a problem hiding this comment.
I reverted this change to <= 0L, this better captures the intention of this: check that the new version is a regular positive one
|
@elasticmachine update branch |
Introducing constants for special ReservedClusterStateMetadata versions to make it easier to trace their usage.