[7.x] Enforce data_frozen for partial searchable snapshot _tier_preference (#71155)#71342
[7.x] Enforce data_frozen for partial searchable snapshot _tier_preference (#71155)#71342dakrone merged 7 commits intoelastic:7.xfrom
Conversation
…lastic#71155) We already set `data_frozen` for partial searchable snapshots when they are mounted (elastic#70786), and automatically convert values other than the frozen role automatically for these snapshots when the metadata is loaded (elastic#71014). This commit makes the `_tier_preference` setting validate that the setting is *only* `data_frozen` when set on a partial searchable snapshot index.
|
Pinging @elastic/es-distributed (Team:Distributed) |
|
@elasticmachine update branch |
| public String get(Settings settings) { | ||
| final Version idxVersion = IndexMetadata.SETTING_INDEX_VERSION_CREATED.get(settings); | ||
| if (idxVersion.onOrAfter(Version.V_7_13_0)) { | ||
| // 7.13.0+ indices can use the original setting value | ||
| return super.get(settings); | ||
| } else if (SearchableSnapshotsConstants.isPartialSearchableSnapshotIndex(settings)) { | ||
| // Partial searchable snapshot indices should be restricted to | ||
| // only data_frozen when reading the setting, or else validation fails. | ||
| return DATA_FROZEN; | ||
| } else { | ||
| // Otherwise pass through to the regular setting retrieval | ||
| return super.get(settings); | ||
| } | ||
| } |
There was a problem hiding this comment.
@henningandersen I ended up having to change this for the backport. I'm not sure yet how I feel about it. Otherwise, a shared_cache searchable snapshot index created in 7.12.1 in a mixed-version cluster (where some clusters are 7.13.0) cause validation failures for the index, since we set it to data_frozen,data_cold,data_warm,data_hot in 7.12.1 and reject that in 7.13.0
Does this seem like a reasonable compromise for that case?
There was a problem hiding this comment.
@dakrone I do not really have a better workaround except to allow an explicit upgrade which does seems a bit excessive. I could be mildly worried about Settings.Builder.put(Settings) copying the underlying map rather than the setting values, meaning a copy would get the full string, but I have a hard time figuring out a scenario where this could cause issues. I would possibly prefer to avoid the version check in 7.x and just always return DATA_FROZEN for frozen indices.
Maybe @rjernst has input to this?
There was a problem hiding this comment.
I think the current code in the PR is the right approach. I don't think copying via settings builder will be an issue, whatever is in the map will be the same in the new map, there won't be any translation (we don't call get to copy the values, since we don't have the appropriate Setting objects).
There was a problem hiding this comment.
Okay, thanks for taking a look Ryan.
@henningandersen I pushed a change to make it unconditional instead of version-checked.
Backports the following commits to 7.x: