Fix Broken Numeric Shard Generations in RepositoryData#57813
Fix Broken Numeric Shard Generations in RepositoryData#57813original-brownbear merged 3 commits intoelastic:masterfrom original-brownbear:broken-repo-data-auto-repair
Conversation
Fix broken numeric shard generations when reading them from the wire or physically from the physical repository. This should be the cheapest way to clean up broken shard generations in a BwC and safe-to-backport manner for now. We can potentially further optimize this by also not doing the checks on the generations based on the versions we see in the `RepositoryData` but I don't think it matters much since we will read `RepositoryData` from cache in almost all cases. Closes #57798
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
|
Nevermind, problematic corner case is actually good I think. |
ywelsch
left a comment
There was a problem hiding this comment.
Thanks Armin, I've left one stylistic comment, and one question
| shardGenerations.put(indexId, i, gens.get(i)); | ||
| final String parsedGen = gens.get(i); | ||
| shardGenerations.put( | ||
| indexId, i, fixBrokenShardGens ? ShardGenerations.fixShardGeneration(parsedGen) : parsedGen); |
There was a problem hiding this comment.
I think we should avoid putting null values into the map (in particular as ShardGenerations.Builder.put does not seem to define @Nullable String generation, and the semantics of putting null into map can be problematic, as the Map interface says the behavior is underspecified whether a NPE is thrown). I would prefer a boolean method here with conditional logic
| XContentParser parser = XContentType.JSON.xContent().createParser(NamedXContentRegistry.EMPTY, | ||
| LoggingDeprecationHandler.INSTANCE, blob)) { | ||
| repositoryData = RepositoryData.snapshotsFromXContent(parser, latestGen); | ||
| repositoryData = RepositoryData.snapshotsFromXContent(parser, latestGen, true); |
There was a problem hiding this comment.
Right my bad no reason to be lenient here ... moved to false.
ywelsch
left a comment
There was a problem hiding this comment.
Should we also get this into 7.7.2 (if we ever going to release that one)?
|
Thanks! @ywelsch about the backport, I was going to ask the same about the other PR as well ... I don't see why not? => I'll backport this and the other one to 7.7.2? |
|
👍 |
Fix broken numeric shard generations when reading them from the wire or physically from the physical repository. This should be the cheapest way to clean up broken shard generations in a BwC and safe-to-backport manner for now. We can potentially further optimize this by also not doing the checks on the generations based on the versions we see in the `RepositoryData` but I don't think it matters much since we will read `RepositoryData` from cache in almost all cases. Closes #57798
Fix broken numeric shard generations when reading them from the wire or physically from the physical repository. This should be the cheapest way to clean up broken shard generations in a BwC and safe-to-backport manner for now. We can potentially further optimize this by also not doing the checks on the generations based on the versions we see in the `RepositoryData` but I don't think it matters much since we will read `RepositoryData` from cache in almost all cases. Closes #57798
Fix broken numeric shard generations when reading them from the wire or physically from the physical repository. This should be the cheapest way to clean up broken shard generations in a BwC and safe-to-backport manner for now. We can potentially further optimize this by also not doing the checks on the generations based on the versions we see in the `RepositoryData` but I don't think it matters much since we will read `RepositoryData` from cache in almost all cases. Closes #57798
|
Hey @original-brownbear it looks like we missed mentioning this PR in the release notes. |
Yea I guess technically that would be correct. Not sure how relevant it is at this point, but technically it's the first version released with this fix. |
Fix broken numeric shard generations when reading them from the wire
or physically from the physical repository.
This seems to me to be the cheapest way to clean up broken shard generations
in a BwC and safe-to-backport manner for now. We can potentially
further optimize this by also not doing the checks on the generations
based on the versions we see in the
RepositoryDatabut I don't thinkit matters much since we will read
RepositoryDatafrom cache in almostall cases.
Closes #57798