add option for _ingest.timestamp to use new ZonedDateTime (5.x backport) #24030
Conversation
talevy
added
:Data Management/Ingest Node
| @@ -51,6 +51,7 @@ | |||
|
|
|||
| private final Pipeline.Factory factory = new Pipeline.Factory(); | |||
| private final Map<String, Processor.Factory> processorFactories; | |||
| private final boolean newDateFormat; | |||
There was a problem hiding this comment.
Maybe rename to newIngestTimestampFormat, since it only applies to ingest timestamp?
There was a problem hiding this comment.
will do, guess I took it for granted that this resides in an ingest-specific class.
| /** | ||
| * Holder class for several ingest related services. | ||
| */ | ||
| public class IngestService { | ||
| public static final Setting<Boolean> NEW_DATE_FORMAT = |
There was a problem hiding this comment.
If my previous comment makes sense then this should be renamed as well.
| /** | ||
| * Holder class for several ingest related services. | ||
| */ | ||
| public class IngestService { | ||
| public static final Setting<Boolean> NEW_DATE_FORMAT = | ||
| Setting.boolSetting("ingest.new_date_format", false, Property.NodeScope, Property.Dynamic, Property.Deprecated); |
There was a problem hiding this comment.
I think we should not set the the dynamic flag here, because this setting isn't actually updated at runtime in PipelineStore (it is an immutable property).
Or instead we should make it a setting that can be setting that can be changed dynamically at runtime. (by using ClusterSettings#addSettingsUpdateConsumer(...) )
There was a problem hiding this comment.
@martijnvg I am a bit new to how settings are managed here. Since we want this setting to be applied to all ingest-nodes, not just one...
is it right to leave it NodeScope+Dynamic and implement the addSettingsUpdateConsumer and keep the setting-flag up-to-date? Having this be non-dynamic and set per-node sounds dangerous since one ingest-node may do one thing, while another does something else, etc.
|
@martijnvg thanks for the review, I've updated it to properly register a settingUpdater, thanks for teaching me about that! |
| @@ -51,16 +52,23 @@ | |||
|
|
|||
| private final Pipeline.Factory factory = new Pipeline.Factory(); | |||
| private final Map<String, Processor.Factory> processorFactories; | |||
| private Boolean newIngestDateFormat; | |||
There was a problem hiding this comment.
This field should be 'volatile now, since it gets updated by a different thread than it is read from.
Maybe also change the type to a primitive boolean?
|
thanks! good catch, also fixed some long-lines |
This is a backport of the update to 6.0 that defaults this new behavior. Previously, Mustache would call `toString` on the `_ingest.timestamp` field and return a date format that did not match Elasticsearch's defaults for date-mapping parsing. The new ZonedDateTime class in Java 8 happens to do format itself in the same way ES is expecting. Fixes elastic#23168. This new fix can be found in the form of a cluster setting called `ingest.new_date_format`. By default, in 5.x, the existing behavior will remain the same. One will set this property to `true` in order to take advantage of this update for ingest-pipeline convenience.
|
rebased to include latest |
|
will merge this into both master and 5.x then follow-up with appropriate documentation updates for |
Previously, Mustache would call `toString` on the `_ingest.timestamp` field and return a date format that did not match Elasticsearch's defaults for date-mapping parsing. The new ZonedDateTime class in Java 8 happens to do format itself in the same way ES is expecting. This commit adds support for a feature flag that enables the usage of this new date format that has more native behavior. Fixes #23168. This new fix can be found in the form of a cluster setting called `ingest.new_date_format`. By default, in 5.x, the existing behavior will remain the same. One will set this property to `true` in order to take advantage of this update for ingest-pipeline convenience.
|
merged change into 5.x: 38ac92a |
* master: Increase compilation limit in ingest tests Mark 6.0.0-alpha1 as prerelease Updated release notes for 6.0.0-alpha1 Fix single shard scroll within a cluster with nodes in version >= 5.3 and <= 5.3 (elastic#24512) add option for _ingest.timestamp to use new ZonedDateTime (elastic#24030) Fixes inefficient loading of snapshot repository data (elastic#24510) Scripting: Deprecate file scripts (elastic#24552) Remove commented code from ESILRTC Ensure test replicas have valid recovery state Add global checkpoint assertion in index shard Improve bootstrap checks error messages Refactor UpdateHelper into unit-testable pieces Fix cache expire after access Document work-around for jar hell in idea_rt.jar file (elastic#24523) Move MockLogAppender to elasticsearch test (elastic#24542) Remove gap skipping when opening engine documentation of preserve existing settings remove duplicated import in AppendProcessor
This is a backport-veresion of the update to 6.0 that defaults this new behavior: #23174.
I'd like to merge this in first, then remove the setting in master as a follow-up
Previously, Mustache would call
toStringon the_ingest.timestampfield and return a date format that did not match Elasticsearch's
defaults for date-mapping parsing. The new ZonedDateTime class in Java 8
happens to do format itself in the same way ES is expecting.
Fixes #23168.
This new fix can be found in the form of a cluster setting called
ingest.new_date_format. By default, in 5.x, the existing behaviorwill remain the same. One will set this property to
truein order totake advantage of this update for ingest-pipeline convenience.