[BUG] DATETIME_FORMATTER_CACHING_SETTING experimental feature should not default to true

[jed326]

Describe the bug

This was first reported on the opensearch forum: https://forum.opensearch.org/t/opensearch-experimental-setting-true-by-default-datetime-formatter-caching/19067

Just wanted to bring up a change that was added into OpenSearch 2.12 which was enabled by default and broke some functionality in our applications.

The experimental feature flag opensearch.experimental.optimization.datetime_formatter_caching.enabled defaults to true, see:

OpenSearch/server/src/main/java/org/opensearch/common/util/FeatureFlags.java

Lines 82 to 86 in 81707dc

	public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
	DATETIME_FORMATTER_CACHING,
	true,
	Property.NodeScope
	);

We clearly state in our release notes that experimental feature flags are disabled by default: https://github.com/opensearch-project/opensearch-build/blob/a7f6bb5901545875d7817f26016a1d6c99c4d8ad/release-notes/opensearch-release-notes-2.12.0.md?plain=1#L24

This was introduced in #9567

Related component

Indexing

To Reproduce

See code block linked in description.

Expected behavior

All experimental feature flags should be disabled by default. Ideally this should be enforced in the code as well.

Additional Details

Plugins
Please list all plugins currently enabled.

Screenshots
If applicable, add screenshots to help explain your problem.

Host/Environment (please complete the following information):

• OS: [e.g. iOS]
• Version [e.g. 22]

Additional context
Add any other context about the problem here.

[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?

[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.

[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).

[jed326]

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

[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.

[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.

[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?

[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.

[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]

[mgodwan]

Reopening this since I've not seen any traction.
I'm still trying to understand what is the actual issue here with the new format.

@reta @StewartWBrown1 I'm still not sure of the actual issue here. Could you please help understand this?

[reta]

@mgodwan please check #13411 (comment)

[mgodwan]

@reta I've gone through the issue and comments, but have not been able to figure out what exactly broke with the change. The new default format is compatible with the legacy one if I understand correctly

[reta]

@reta I've gone through the issue and comments, but have not been able to figure out what exactly broke with the change. The new default format is compatible with the legacy one if I understand correctly

@mgodwan as user reported, this is not about functional format compatibility, but format of the datetime field:

[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]. 

I don't know exact use case but I could imaging one, when the existing mappings for the index were created with the "old" default but than when the "new" default kicks in, it triggers an attempt to update the existing mapping (please notice, no changes on user side).

[mgodwan]

when the existing mappings for the index were created with the "old" default but than when the "new" default kicks in

In case of default format, mapping format is not persisted ever.
Based on my dive deep into the mapping flow for datetime field, this can only happen when a user tries to explicitly applies the format which they expect to be the default.

e.g.

Create index with https://github.com/opensearch-project/opensearch-benchmark-workloads/blob/main/so/index.json


Try

curl --location --request PUT 'localhost:9200/so/_mappings' \
--header 'Content-Type: application/json' \
--data-raw '{
    "properties": {
      "creationDate": {
        "type": "date",
        "format": "strict_date_optional_time||epoch_millis"
      }
    }
}'
{"acknowledged":true}

Till 2.11, the above would've passed. Starting 2.12, user would've seen the error reported above. I don't think that this accounts for breaking change (as ideally, we should not even allow setting an explicit format when default is used but sadly opensearch had been allowing this).

[reta]

Thanks, this is clear now.

I don't think that this accounts for breaking change (as ideally, we should not even allow setting an explicit format when default is used but sadly opensearch had been allowing this).

This is 100% breaking change for user: the feature that was worked before is not anymore (without any changes from user side).

we should not even allow setting an explicit format when default is used but sadly opensearch had been allowing this).

Since the default could change (which we sadly did), why we should not even allow setting an explicit format?

[StewartWBrown1]

@mgodwan In the documentation itself, OpenSearch states:

Reviewing breaking changes
It’s important to determine how the new version of OpenSearch will integrate with your environment. Review Breaking changes before beginning any upgrade procedures to determine whether you will need to make adjustments to your workflow. For example, upstream or downstream components might need to be modified to be compatible with an API change

In this instance, upstream/downstream components were affected by an API change due to the new default datetime setting, and therefore in my view this can be considered a breaking change. The Upgrade docs also state that 'breaking changes are only introduced between major version releases.' and this was a minor version bump from 2.11.0 -> 2.12.0

My original forum post was to point out this change did break us & wasn't documented in the upgrade notes, but also to point out this change occurred through a new setting tagged as 'experimental' which the documentation also says should be 'false' by default. With both these things in mind, I agree with the discussion on this ticket and think it should be 'false' by default.

[mgodwan]

Since the default could change (which we sadly did), why we should not even allow setting an explicit format?

@reta In general, we don't allow mappings for a field to be modified (e.g. doc_values cannot be enabled for a field for which it has been disabled). Similar to that, we don't allow date_time format to be changed once set (e.g. you cannot change from epoch_millis to strict_data_time_no_millis) . Only in case of default, users can set it explicitly equal to the backing default (and not to any other format) which is the payload which broke.

[mgodwan]

@StewartWBrown1 Yes, the change can be considered breaking in line with the semantic versioning guidelines followed for OpenSearch project and I'm ok keeping it false. Could you help me understand the use-case due to which you need to set the format explicitly here and could not rely on the new backing default? I don't think indexing would have broken as the new default format is backward compatible. I'm thinking we should not allow updating format in any case today and would like to open another discussion on the path to make the new experimental default format generally available without a feature flag.

wasn't documented in the upgrade notes,

The documentation was updated here https://opensearch.org/docs/latest/field-types/supported-field-types/date/#default-format . I'm unsure why it never made to release notes.

[reta]

@reta In general, we don't allow mappings for a field to be modified (e.g. doc_values cannot be enabled for a field for which it has been disabled).

@mgodwan My understanding from your comment #13411 (comment) the mapping was not modified but created the first time (and it worked before change and not after)? Modification restrictions are not specific to date format, as you mentioned

[mgodwan]

@reta No, you can successfully create the index. Its only when attempting to modifying the mappings that this error will come, which is the same behaviour as before based on how we look at it (i.e. changing from something other than default is not allowed)

Let me know your thoughts.

% curl -XPUT "http://localhost:9200/index-test" -H 'Content-Type: application/json' -d'
{
  "mappings": {
    "properties": {
      "datetime": {
        "type": "date"
      }
    }
  },
  "settings": {
    "number_of_replicas": 0,
    "number_of_shards": 1
  }
}'
{"acknowledged":true,"shards_acknowledged":true,"index":"index-test"}

% curl -XPUT "http://localhost:9200/index-test/_mappings" -H 'Content-Type: application/json' -d'
{
  "properties": {
    "datetime": {
        "type": "date",
        "format": "strict_date_optional_time||epoch_millis"
      }
  }
}'
{"error":{"root_cause":[{"type":"illegal_argument_exception","reason":"Mapper for [datetime] conflicts with existing mapper:\n\tCannot update parameter [format] from [strict_date_time_no_millis||strict_date_optional_time||epoch_millis] to [strict_date_optional_time||epoch_millis]"}],"type":"illegal_argument_exception","reason":"Mapper for [datetime] conflicts with existing mapper:\n\tCannot update parameter [format] from [strict_date_time_no_millis||strict_date_optional_time||epoch_millis] to [strict_date_optional_time||epoch_millis]"},"status":400}

---

% curl -XPUT "http://localhost:9200/index-test/_mappings" -H 'Content-Type: application/json' -d'
{
  "properties": {
    "datetime": {
        "type": "date",
        "format": "strict_date_time_no_millis||strict_date_optional_time||epoch_millis"
      }
  }
}'
{"acknowledged":true}



----
TRY CREATING A NEW INDEX WITH OLD DEFAULT

% curl -XPUT "http://localhost:9200/index-test-1" -H 'Content-Type: application/json' -d'
{
  "mappings": {
    "properties": {
      "datetime": {
        "type": "date",
        "format": "strict_date_optional_time||epoch_millis"
      }
    }
  },
  "settings": {
    "number_of_replicas": 0,
    "number_of_shards": 1
  }
}'
{"acknowledged":true,"shards_acknowledged":true,"index":"index-test-1"}

[reta]

@reta No, you can successfully create the index. Its only when attempting to modifying the mappings that this error will come, which is the same behaviour as before based on how we look at it (i.e. changing from something other than default is not allowed)

Thanks @mgodwan , so it is exactly the flow I suspected (#13411 (comment)). In any case, I strongly lean towards classifying this change as a breaking: we do allow "noop" mapping updates but the change of the default date format broke that.

[mgodwan]

so it is exactly the flow I suspected (#13411 (comment)).

Just to clarify on the part it triggers an attempt to update the existing mapping (please notice, no changes on user side). from the linked comment, this does not happen within OpenSearch but only through a user action, and can be classified as breaking for users attempting to perform this.

(I honestly don't see a use case for performing this action but I agree that this may not be in line with semantic versioning guidelines and hence may justify the revert. I've asked the original reporter about the use case as well so that it gives us a path to solve for the issue and decide if we can disable no-op format modifications for the user as it is kind of leaking an implementation detail through the API)

• I would like to brainstorm further on how to enable the new default format (given the performance gains highlighted in the original issue). Do you think this can be done for indices created on new versions at least (e.g. apply the defaults only for index.created.version >= 2.15.0) or would that be considered breaking as well?
• Also, would like to hear community thoughts on if we should disable modifying from default to explicit format (i.e. no-op) from 3.0 through a validation check in DateFieldMapper?

[reta]

I would like to brainstorm further on how to enable the new default format (given the performance gains highlighted in the original issue).

I think we have to fix this feature rollout by introducing 2 separate feature flags (I think @CaptainDredge mentioned that as well at some point):

• the caching one DATETIME_FORMATTER_CACHING_SETTING (current, which you probably interested in since it is focused on performance gains)
• the caching one DATETIME_FORMATTER_DEFAULT_SETTING (new, which does default format change)

[reta]

@mgodwan I am closing this issue as per resolution, DATETIME_FORMATTER_CACHING_SETTING is now false and delivered in 2.14.0, please feel free to open follow up issues, we have quite a few options to proceed, thank you.