-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Throw better exception if verifying empty repo #131677
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
Throw better exception if verifying empty repo #131677
Conversation
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message.
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
Hi @DaveCTurner, I've created a changelog YAML for you. |
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
|
||
static void ensureValidGenId(long repositoryGenId) { | ||
if (repositoryGenId == RepositoryData.EMPTY_REPO_GEN) { | ||
throw new IllegalArgumentException("repository is empty, cannot verify its integrity"); |
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.
Nit: I think there is "no need" to verify its integrity instead of "cannot". The later is technically correct due to the negative number. But the former reads more friendly and helpful. It might even be OK to return empty response here. But an exception seems slightly more preferrable.
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 know what you mean, but my rationale for using "cannot" here is that the user has probably made a mistake with their repository config to get to this point (e.g. incorrect bucket or base path) - that seems much more likely than a user just wanting to check a totally empty bucket for integrity violations.
💔 Backport failed
You can use sqren/backport to manually backport by running |
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of elastic#131677 to `9.1`
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of elastic#131677 to `9.0`
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of elastic#131677 to `8.19`
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of elastic#131677 to `8.18`
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of #131677 to `9.0`
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of #131677 to `8.19`
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of #131677 to `8.18`
Today if you attempt to verify the integrity of a brand-new repository (no `index-${N}` blob) then it will fail because the repository generation is `-1` which cannot be sent over the wire. But it makes no sense to verify the integrity of such a repository anyway, so with this commit we fail such requests up-front with a more helpful error message. Backport of #131677 to `9.1`
Today if you attempt to verify the integrity of a brand-new repository
(no
index-${N}
blob) then it will fail because the repositorygeneration is
-1
which cannot be sent over the wire. But it makes nosense to verify the integrity of such a repository anyway, so with this
commit we fail such requests up-front with a more helpful error message.