Fix unknown camel case date-time mappings #88400
Conversation
grcevski
added
>bug
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @grcevski, I've created a changelog YAML for you. |
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public enum LegacyFormatNames { |
There was a problem hiding this comment.
Copied back from 7.17, with removed unnecessary methods.
There was a problem hiding this comment.
While this fixes the immediate issue, I don't think it addresses the underlying problem: these format names exist in the mappings, which are in cluster state. So with this PR as is, a user would be able to upgrade to 8.x, and their mappings would work. But they would still show up with camelCase, and nothing would indicate to them they need to change that. Could we instead change the mappings themselves on upgrade, on the master node as we load cluster state? ie similar to other mappings upgrades we have done.
| YEAR_MONTH_DAY("yearMonthDay", "year_month_day"), | ||
| EPOCH_SECOND(null, "epoch_second"), | ||
| EPOCH_MILLIS(null, "epoch_millis"), | ||
| // strict date formats here, must be at least 4 digits for year and two for months and two for day" |
| this.snakeCaseName = snakeCaseName; | ||
| } | ||
|
|
||
| public static String compatibleFormat(String format) { |
There was a problem hiding this comment.
maybe use "get" terminology here? getCompatibleFormat
|
relates #58719 |
| this.format = Parameter.stringParam( | ||
| "format", | ||
| indexCreatedVersion.isLegacyIndexVersion(), | ||
| (n, c, o) -> LegacyFormatNames.compatibleFormat(XContentMapValues.nodeStringValue(o)), |
There was a problem hiding this comment.
just wanted to clarify. So LegacyFormatNames.compatibleFormat is returning a snake case for given camel case?
Would it be worth renaming to make this more clear?
| @@ -197,6 +197,7 @@ public Set<Entry<String, NamedAnalyzer>> entrySet() { | |||
| mapperService.merge(indexMetadata, MapperService.MergeReason.MAPPING_RECOVERY); | |||
| } | |||
| } catch (Exception ex) { | |||
| logger.error("Failed to parse mappings for index [" + indexMetadata.getIndex() + "]", ex); | |||
There was a problem hiding this comment.
might be a dirty hack, but people on #84199 were tweaking the exception below to be null just to make upgrade/downgrade possible.
Maybe we could check what the exception is and if it was due to camelcase format we could return null as well?
I did not tested this though.
but at the same time, maybe your fix is preventing this exception from happening?
There was a problem hiding this comment.
I can remove the logger error, it was very hard to see why things fail without it, the original exception is hidden. My fix now prevents the exception to be thrown, but if we encountered one again I think we'll be in the same boat, not seeing what the actual error is.
|
I pushed a change to see if BWC tests pass with my code to fix up the mappings. I still need to write tests. |
|
Tests for the upgrader are all done now. |
| @@ -90,7 +90,7 @@ public IndexMetadata verifyIndexMetadata(IndexMetadata indexMetadata, Version mi | |||
| // Next we have to run this otherwise if we try to create IndexSettings | |||
| // with broken settings it would fail in checkMappingsCompatibility | |||
| newMetadata = archiveBrokenIndexSettings(newMetadata); | |||
| checkMappingsCompatibility(newMetadata); | |||
| newMetadata = checkMappingsCompatibility(newMetadata); | |||
There was a problem hiding this comment.
Can we add a new method for doing the transformation? The "check" action to me implies nothing will change, it's just checking things.
There was a problem hiding this comment.
It's only used here, so I'll simply rename the method and update the comments.
| defaultFormat.pattern() | ||
| ); | ||
|
|
||
| if (indexCreatedVersion.major <= Version.CURRENT.previousMajor().major) { |
There was a problem hiding this comment.
Should this version be a fixed constant? ie why not hardcoded to 8, or use one of the V_8_* constants (maybe 8.0.0?). Otherwise when the major version bumps this would start applying to 8.x.
| /** | ||
| * Upgrades the format field of mapping properties | ||
| * <p> | ||
| * This method checks is the new mapping source has a different format than the original mapping, and if it |
| } | ||
|
|
||
| try { | ||
| return new CompressedXContent(sourceMap); |
There was a problem hiding this comment.
Could you keep track of whether any change was made above, so that the original parsedSource can be returned here and reference equality could be used by the caller to detect changes? That would make it less expensive when there aren't any changes (which is most of the time).
| Map<String, Object> parsedProperties = getMappingProperties(newMap, mappingMetadata.type()); | ||
| if (properties != null && parsedProperties != null) { | ||
| for (var property : properties.entrySet()) { | ||
| if (property.getValue()instanceof Map map) { |
There was a problem hiding this comment.
Will this handle object fields? It seems like it would only handle date fields at the root of mappings.
There was a problem hiding this comment.
Ah yeah I forgot about nested field mappings. I need to fix that.
| * @param parsedSource the mapping source as it was parsed | ||
| */ | ||
| @SuppressWarnings({ "unchecked", "rawtypes" }) | ||
| public static CompressedXContent upgradeFormatsIfNeeded(MappingMetadata mappingMetadata, CompressedXContent parsedSource) { |
There was a problem hiding this comment.
This method name implies it is a general upgrade, but isn't this specific to date fields, since you are looking for the format key?
rjernst
added
the
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
| (Map<String, Object>) parsedMappings.get("properties") | ||
| ); | ||
|
|
||
| boolean anyRuntimeChanges = upgradeDateFormatProperties( |
There was a problem hiding this comment.
I didn't do anything here for dynamic_templates, they don't seem to trigger a parse of a Date field in the same way it's happening for 'properties' and 'runtime'.
|
Closing in favour of #88914 |
Elasticsearch 8.0 dropped support for camel case date time mappings, however we still support 7.x indices in 8.0. Some of these 7.x indices might've been created with camel case mappings, even though those were deprecated in 7.x, nothing stopped users from using them.
When we encounter a mapping with camel case on a 7.x index in 8.0 we mess up the cluster data, because the validation checks run way too late, after we've effectively upgraded the metadata and the directories structure.
This PR brings back the camel case compatibility for 7.x compatible indices in 8.0, the option to use a camel case index doesn't exist anymore, but we'll allow the 7.x index to work by translating the parsed field mapping.
Closes #84199