[BUG] DATETIME_FORMATTER_CACHING_SETTING experimental feature should not default to true about opensearch

[jed326]

I've asked the original poster on the forum for some more details on what specifically broke for them

[reta]

Thanks @reta -- do you think a static setting would make sense here then?

I mean there is no access to any kind of settings when the formatters are initialized. Reverting the default to false as it should have been done would probably makes sense.

[andrross]

I don't think this change would have any impact on existing apps (if the switch between default date formatters was not noticeable, it still won't be noticeable either)

@reta Agree, I think we should change the default back to the pre-2.11 behavior since we know this change was breaking for at least one user. I suspect more people (who have yet to upgrade to 2.11+) will be impacted if we don't fix this ASAP.

[jed326]

I also agree that changing back to false is the right way to do it! As a tangent I wonder if there's anything we can do to enforce this in the code itself -- will briefly explore that and open a separate issue if I find anything.

[jed326]

Tagging folks who worked on original PR: @reta @CaptainDredge @ankitkala

It seems like this PR has been backported all the way to 2.11 which makes me think this isn't really an experimental feature flag anymore, which in that case we should move this to a dynamic setting.

[reta]

It seems like this PR has been backported all the way to 2.11 which makes me think this isn't really an experimental feature flag anymore, which in that case we should move this to a dynamic setting.

Thanks a lot @jed326 for bringing this one up, I think yes, we messed it up with default I believe, it indeed should be set as false as experimental setting. The true came out of the good intents, but it is not aligned with the semantic of the experimental setting, I agree.

AFAIK we cannot make it dynamic setting - the formatters are static instances that are created with the caching turned on/off, probably we could make it work generally speaking it but it would need redesign, 💯

[jed326]

Thanks @reta -- do you think a static setting would make sense here then? I know we have a 2.14 release coming up soon but too, do you see this as a release blocker for that?

[peternied]

[Triage - attendees 1 2 3 4 5 6 7 8]
@jed326 Thanks for creating this issue.

[CaptainDredge]

Reverting the default to false as it should have been done would probably makes sense.

@reta this would then result in a change of default format in the new version. How about we take the feature out of experimental setting? I feel like since this was enabled by default, sufficient bake time has been provided for any issue to bubble up. Wdyt?

[reta]

@reta this would then result in a change of default format in the new version.

@CaptainDredge Hm .. why would default format change? this is for caching only, right?

I feel like since this was enabled by default, sufficient bake time has been provided for any issue to bubble up. Wdyt?

The takeaway from this issue is that it is turned off by some users, we cannot make it non-experimental without clearing the behaviour first.

[jed326]

@reta This does change the default format:

OpenSearch/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java

Lines 105 to 109 in 0ddbd96

	public static DateFormatter getDefaultDateTimeFormatter() {
	return FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING)
	? DEFAULT_DATE_TIME_FORMATTER
	: LEGACY_DEFAULT_DATE_TIME_FORMATTER;
	}

I do think what @CaptainDredge suggested in moving this out of experimental is where we eventually want to end up. It seems like the main sentiment in the forum post that reported this is that there's uncertainty in what will be default going forward since this is currently under experimental.

[reta]

@reta This does change the default format:

Ah yeah, thanks @jed326 , even the setting is somewhat confusing than, I think @CaptainDredge suggesting would make sense

[andrross]

It seems like the main sentiment in the forum post...is that there's uncertainty

@jed326 @CaptainDredge The forum post says "...was enabled by default and broke some functionality in our applications". Have we dug into that? We should seriously consider reverting the default behavior back to the pre 2.11 format since we've got at least one data point that this is a breaking change.

[reta]

@jed326 @CaptainDredge The forum post says "...was enabled by default and broke some functionality in our applications"

Thanks @andrross , I missed the "broke" part but it should be included into this issue in bold, I suspect the default format change could be the culprit here (caching is less likely but possible).

[CaptainDredge]

The takeaway from this issue is that it is turned off by some users, we cannot make it non-experimental without clearing the behaviour first

Definitely! Ideally the behaviour should've remained same since the new default format is just an extension of old default format. I'm highly curious to know what broke for the user in order to understand the functional difference between the old and new format

[StewartWBrown1]

Hey all, yeah the error we were seeing after upgrading to OpenSearch 2.12 was:

[REQUEST_HANDLING_ERROR_CAUSED_BY] Caused by: exception: OpenSearchException message: OpenSearch returned an error: Code: 400. Error: illegal_argument_exception:Mapper for [created_at_dttm] conflicts with existing mapper: Cannot update parameter [format] from [strict_date_time_no_millis||strict_date_optional_time||epoch_millis] to [strict_date_optional_time||epoch_millis].

Guess this was because in several instances a type mismatch between how we were formatting our dates, and the default change in the new OpenSearch. Everything was working again as expected when we disabled the default format change with the experimental flag.

As @jed326 mentioned above, main motivation for forum post was to see whether this would be default in future, and to bring up the oddity that an 'experimental' feature was enabled by default.

Also, took a little bit of time to properly figure out this change too, as nothing to do with the default datetime format changing was mentioned in the release note that I could see.

Cheers for raising this issue and the discussion around it! :)

[reta]

Thanks @StewartWBrown1 , @andrross @jed326 @CaptainDredge I strongly feel like we have to step back and follow the proper rollout process:

• keep setting experimental
• make false to be a default value (as it should have been, we sadly overlooked that)

I don't think this change would have any impact on existing apps (if the switch between default date formatters was not noticeable, it still won't be noticeable either). But it would address user's concern, the evidence we've been looking for to confirm that things do break.

[reta]

Thanks @andrross , I think we could make it to 2.14.0 once everyone is on board.

[andrross]

@jed326 @CaptainDredge what do you think?

[mgodwan]

I agree that this is a miss for default.

I believe the indexing would have still gone through for these use cases.

• What is the use case to update the mappings to use explicit format instead of continuing to rely on default format (which is backward compatible with the older one in terms of handling documents)?
• Would a mitigation to update the mappings to use the new default format strict_date_time_no_millis||strict_date_optional_time||epoch_millis helped here (instead of the older default strict_date_optional_time||epoch_millis) due to which 400 error is currently seen?

[Keeping the issue closed but would like to understand the scenario more]