Gate reading of optional string array for bwc#106878
Merged
dnhatn merged 20 commits intoelastic:mainfrom Mar 29, 2024
Merged
Conversation
Collaborator
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
martijnvg
reviewed
Mar 28, 2024
...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/DownsampleShardTaskParams.java
Outdated
Show resolved
Hide resolved
kkrik-es
reviewed
Mar 28, 2024
...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/DownsampleShardTaskParams.java
Outdated
Show resolved
Hide resolved
...ugin/downsample/src/main/java/org/elasticsearch/xpack/downsample/DownsampleShardIndexer.java
Outdated
Show resolved
Hide resolved
Contributor
Author
|
Note that this issue as a side effect described here #106880 |
Collaborator
|
Hi @salvatore-campagna, I've created a changelog YAML for you. |
martijnvg
approved these changes
Mar 28, 2024
Member
martijnvg
left a comment
There was a problem hiding this comment.
I left one question otherwise LGTM
...ugin/downsample/src/main/java/org/elasticsearch/xpack/downsample/DownsampleShardIndexer.java
Show resolved
Hide resolved
...n/downsample/src/main/java/org/elasticsearch/xpack/downsample/DownsampleShardTaskParams.java
Show resolved
Hide resolved
mark-vieira
reviewed
Mar 28, 2024
x-pack/plugin/downsample/qa/mixed-cluster/src/javaRestTest/resources/tsdb-bulk-request.txt
Outdated
Show resolved
Hide resolved
mark-vieira
approved these changes
Mar 29, 2024
dnhatn
pushed a commit
to dnhatn/elasticsearch
that referenced
this pull request
Mar 29, 2024
Missing a check on the transport version results in unreadable cluster state if it includes a serialized instance of DownsampleShardTaskParams. serie indices. Reading an optional array requires reading a boolean first which is required to know if an array of values exists in serialized form. From 8.13 on we try to read such a boolean which is not there because older versions don't write any boolean nor any string array. Here we include the check on versions for backward compatibility skipping reading any boolean or array whatsoever whenever not possible. Customers using downsampling might have cluster states including such serielized objects and would be unable to upgrade to version 8.13. They will be able to upgrade to any version including this fix. This fix has a side effect elastic#106880
elasticsearchmachine
pushed a commit
that referenced
this pull request
Mar 29, 2024
…06896) Missing a check on the transport version results in unreadable cluster state if it includes a serialized instance of DownsampleShardTaskParams. serie indices. Reading an optional array requires reading a boolean first which is required to know if an array of values exists in serialized form. From 8.13 on we try to read such a boolean which is not there because older versions don't write any boolean nor any string array. Here we include the check on versions for backward compatibility skipping reading any boolean or array whatsoever whenever not possible. Customers using downsampling might have cluster states including such serielized objects and would be unable to upgrade to version 8.13. They will be able to upgrade to any version including this fix. This fix has a side effect #106880 Co-authored-by: Salvatore Campagna <93581129+salvatore-campagna@users.noreply.github.com>
Member
|
Backported to 8.13 in #106896 |
This was referenced Mar 29, 2024
kkrik-es
reviewed
Mar 29, 2024
| } | ||
|
|
||
| if (dimensions.length == 0) { | ||
| logger.debug("extracting dimensions from legacy tsid"); |
Member
There was a problem hiding this comment.
I think there are two dimensions here:
- Older index, using legacy tsid. This is controlled by IndexVersion.
- Older transport layer, passing no dimensions. This is controlled by TransportVersion.
It seems possible to have a mix of older indexes with newer transport, as well as newer indexes with older transport. If so, can this fail in case we use a newer index (created in 8.13) with an updated tsid, through an older persistent task? @salvatore-campagna thoughts?
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
Missing a check on the transport version results in unreadable cluster state
if it includes a serialized instance of
DownsampleShardTaskParams.#98023 introduced an optional string array including dimensions used by time
serie indices.
Reading an optional array requires reading a boolean first which is required to
know if an array of values exists in serialized form. From 8.13 on we try to
read such a boolean which is not there because older versions don't write any
boolean nor any string array. Here we include the check on versions for backward
compatibility skipping reading any boolean or array whatsoever whenever not possible.
Customers using downsampling might have cluster states including such serielized
objects and would be unable to upgrade to version 8.13. They will be able to
upgrade to any version including this fix.
This fix has a side effect #106880