Reinstate RepositoryData BwC #100405
Reinstate RepositoryData BwC #100405
Conversation
This commit moves back to using a `"major.minor.patch"` string for the version field in snapshots stored in `RepositoryData`, using the marker string `"8.11.0"` to allow older versions to filter out newer snapshots and adding a new `index_version` field alongside. This format is fully backwardly-compatible, except that it trips an assertion in the versions of 8.10.x released today. When running without assertions enabled, things work correctly in all versions. Relates elastic#98454
DaveCTurner
added
>bug
:Distributed Coordination/Snapshot/Restore
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
This is the substantial part of the fix for #98454. It is independent of #100401 as written, but once #100401 is merged and backported to 8.10 we will be able to re-enable assertions in the bwc tests against newer 8.10.x versions. I've opened it here against |
| if (version != null) { | ||
| if (version.before(IndexVersion.V_8_9_0)) { | ||
| builder.field(VERSION, Version.fromId(version.id()).toString()); | ||
| if (version != null && version.id() != NUMERIC_INDEX_VERSION_MARKER.id()) { |
There was a problem hiding this comment.
Would we not want to retain the version field if it is 8.11.0? I think this nulls it out, which would trigger an old version to read the details out I think.
Definitely a minor point (would require 8.11+ writing data, then 8.9 roundtrip, then 8.11+ roundtrip and finally 8.9 read to be a problem), but seems slightly better anyway. On the other hand, I am sure there is a good reason for this that I have not realized?
There was a problem hiding this comment.
Yes in fact this does not especially matter since we will re-read the SnapshotInfo either way. I added this to help with forensics a little: if we encounter a RepositoryData in the wild with "version":"8.11.0" and no "index_version" field alongside it then we know that it must have been written by a version before 8.11.0.
There was a problem hiding this comment.
Ok. So IIUC, 8.9 will parse the snapshot info's version as something like 8.50.00 and be happy about it.
Would it then be simpler to always null out the version field always (in 8.11+) and only write the index_version field? Any prior version would then read the details from snapshot info.
There was a problem hiding this comment.
I was wondering the exact same thing (why we need version.id() != NUMERIC_INDEX_VERSION_MARKER.id()).
So 8.9 should keep "8.11" as a string version, but will clear out the numeric id out in the roundtrip; then 8.11 will read this (OK). When it's time for 8.11 to write, this if should not be passing, so no version or index_version will be written out.
But I assume you are saying this does not matter, because version will have be updated before writing (will not be 8.11 anymore, but something like 8_500_003)?
There was a problem hiding this comment.
If the version is null when reading, we read it from the snapshot info, also in prior versions. SnapshotInfo uses (and used) and int as xcontent serialization so a new version 8_500_003 would be parsed as 8.50.03 by some old ES version and thus be parseable (but ofc not restorable).
| if (version.before(IndexVersion.V_8_9_0)) { | ||
| builder.field(VERSION, Version.fromId(version.id()).toString()); | ||
| if (version != null && version.id() != NUMERIC_INDEX_VERSION_MARKER.id()) { | ||
| if (version.onOrAfter(IndexVersion.V_8_500_000)) { |
There was a problem hiding this comment.
Perhaps I'm missing something, but I thought the consensus reached was that this comparison should be on minVersion since that is effectively the format version of the entire repository data?
There was a problem hiding this comment.
That is not a problem with this approach, since the NUMERIC_INDEX_VERSION_MARKER_STRING is compatible with older releases.
There was a problem hiding this comment.
Yes, technically we're not changing the format version of the entire repository data here, we're just refining the format of the snapshot version slightly. The format of older version numbers remains unchanged, newer version numbers are formatted such that older versions will see them as 8.11.0, but newer versions will interpret them correctly.
In fact the round-tripping problem doesn't matter here, because the authoritative source of a snapshot version is the SnapshotInfo file, and we already have a mechanism to re-read these files if we find we do not have everything we expect.
|
#100447 is effectively the same change, but against |
This commit moves back to using a
"major.minor.patch"string for theversion field in snapshots stored in
RepositoryData, using the markerstring
"8.11.0"to allow older versions to filter out newer snapshotsand adding a new
index_versionfield alongside.This format is fully backwardly-compatible, except that it trips an
assertion in the versions of 8.10.x released today. When running without
assertions enabled, things work correctly in all versions.
Relates #98454