Refactor DateTimeFormatter Access under featireflag

Signed-off-by: Prabhat Sharma <[email protected]>

distribution/src/config/opensearch.yml

@@ -126,4 +126,4 @@ ${path.logs}
 # Gates the optimization of datetime formatters caching along with change in default datetime formatter
 # Once there is no observed impact on performance, this feature flag can be removed.
 #
-#opensearch.experimental.optimization.datetime_formatter_caching.enabled: true
+#opensearch.experimental.optimization.datetime_formatter_caching.enabled: false

server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/DateHistogramIT.java

@@ -124,7 +124,7 @@ protected Settings featureFlagSettings() {
     }
 
     private ZonedDateTime date(String date) {
-        return DateFormatters.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse(date));
+        return DateFormatters.from(DateFieldMapper.getDefaultDateTimeFormatter().parse(date));
     }
 
     private static String format(ZonedDateTime date, String pattern) {
@@ -1624,8 +1624,8 @@ public void testScriptCaching() throws Exception {
                 .setSettings(Settings.builder().put("requests.cache.enable", true).put("number_of_shards", 1).put("number_of_replicas", 1))
                 .get()
         );
-        String date = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(date(1, 1));
-        String date2 = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(date(2, 1));
+        String date = DateFieldMapper.getDefaultDateTimeFormatter().format(date(1, 1));
+        String date2 = DateFieldMapper.getDefaultDateTimeFormatter().format(date(2, 1));
         indexRandom(
             true,
             client().prepareIndex("cache_test_idx").setId("1").setSource("d", date),

server/src/internalClusterTest/java/org/opensearch/search/aggregations/bucket/DateHistogramOffsetIT.java

@@ -92,7 +92,7 @@ protected Settings featureFlagSettings() {
     }
 
     private ZonedDateTime date(String date) {
-        return DateFormatters.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse(date));
+        return DateFormatters.from(DateFieldMapper.getDefaultDateTimeFormatter().parse(date));
     }
 
     @Before

server/src/main/java/org/opensearch/common/time/JavaDateFormatter.java

@@ -52,6 +52,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.function.BiConsumer;
 import java.util.stream.Collectors;
 
@@ -149,7 +150,7 @@ JavaDateFormatter getRoundupParser() {
         if (parsers.length == 0) {
             this.parsers = Collections.singletonList(printer);
         } else {
-            this.parsers = Arrays.asList(parsers);
+            this.parsers = new CopyOnWriteArrayList<>(parsers);
         }
         List<DateTimeFormatter> roundUp = createRoundUpParser(format, roundupParserConsumer);
         this.roundupParser = new RoundUpFormatter(format, roundUp);
@@ -218,7 +219,7 @@ public static DateFormatter combined(
             printer,
             roundUpParsers,
             parsers,
-            canCacheLastParsedFormatter & FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING)
+            canCacheLastParsedFormatter & FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING)
         ); // check if caching is enabled
     }
 
@@ -234,7 +235,7 @@ private JavaDateFormatter(
         this.printFormat = printFormat;
         this.printer = printer;
         this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers) : null;
-        this.parsers = parsers;
+        this.parsers = new CopyOnWriteArrayList<>(parsers);
         this.canCacheLastParsedFormatter = canCacheLastParsedFormatter;
     }
 
@@ -296,8 +297,10 @@ private TemporalAccessor doParse(String input) {
             }
             if (lastParsedformatter != null) {
                 if (canCacheLastParsedFormatter && lastParsedformatter != parsers.get(0)) {
-                    parsers.remove(lastParsedformatter);
-                    parsers.add(0, lastParsedformatter);
+                    synchronized (parsers) {
+                        parsers.remove(lastParsedformatter);
+                        parsers.add(0, lastParsedformatter);
+                    }
                 }
                 return (TemporalAccessor) object;
             }
@@ -316,7 +319,9 @@ public DateFormatter withZone(ZoneId zoneId) {
         if (zoneId.equals(zone())) {
             return this;
         }
-        List<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList());
+        List<DateTimeFormatter> parsers = new CopyOnWriteArrayList<>(
+            this.parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList())
+        );
         List<DateTimeFormatter> roundUpParsers = this.roundupParser.getParsers()
             .stream()
             .map(p -> p.withZone(zoneId))
@@ -330,7 +335,9 @@ public DateFormatter withLocale(Locale locale) {
         if (locale.equals(locale())) {
             return this;
         }
-        List<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList());
+        List<DateTimeFormatter> parsers = new CopyOnWriteArrayList<>(
+            this.parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList())
+        );
         List<DateTimeFormatter> roundUpParsers = this.roundupParser.getParsers()
             .stream()
             .map(p -> p.withLocale(locale))

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

@@ -88,6 +88,17 @@ public static boolean isEnabled(String featureFlagName) {
         return settings != null && settings.getAsBoolean(featureFlagName, false);
     }
 
+    public static boolean isEnabled(Setting<Boolean> featureFlag) {
+        if ("true".equalsIgnoreCase(System.getProperty(featureFlag.getKey()))) {
+            // TODO: Remove the if condition once FeatureFlags are only supported via opensearch.yml
+            return true;
+        } else if (settings != null) {
+            return featureFlag.get(settings);
+        } else {
+            return featureFlag.getDefault(Settings.EMPTY);
+        }
+    }
+
     public static final Setting<Boolean> SEGMENT_REPLICATION_EXPERIMENTAL_SETTING = Setting.boolSetting(
         SEGMENT_REPLICATION_EXPERIMENTAL,
         false,

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

@@ -52,6 +52,7 @@
 import org.opensearch.common.time.DateMathParser;
 import org.opensearch.common.time.DateUtils;
 import org.opensearch.common.unit.TimeValue;
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.common.util.LocaleUtils;
 import org.opensearch.index.fielddata.IndexFieldData;
 import org.opensearch.index.fielddata.IndexNumericFieldData.NumericType;
@@ -101,6 +102,12 @@ public final class DateFieldMapper extends ParametrizedFieldMapper {
         "strict_date_optional_time"
     );
 
+    public static DateFormatter getDefaultDateTimeFormatter() {
+        return FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING)
+            ? DEFAULT_DATE_TIME_FORMATTER
+            : LEGACY_DEFAULT_DATE_TIME_FORMATTER;
+    }
+
     /**
      * Resolution of the date time
      *
@@ -231,13 +238,13 @@ public static class Builder extends ParametrizedFieldMapper.Builder {
             "format",
             false,
             m -> toType(m).format,
-            DEFAULT_DATE_TIME_FORMATTER.pattern()
+            getDefaultDateTimeFormatter().pattern()
         );
         private final Parameter<String> printFormat = Parameter.stringParam(
             "print_format",
             false,
             m -> toType(m).printFormat,
-            DEFAULT_DATE_TIME_FORMATTER.printPattern()
+            getDefaultDateTimeFormatter().printPattern()
         ).acceptsNull();
         private final Parameter<Locale> locale = new Parameter<>(
             "locale",
@@ -365,15 +372,15 @@ public DateFieldType(
         }
 
         public DateFieldType(String name) {
-            this(name, true, false, true, DEFAULT_DATE_TIME_FORMATTER, Resolution.MILLISECONDS, null, Collections.emptyMap());
+            this(name, true, false, true, getDefaultDateTimeFormatter(), Resolution.MILLISECONDS, null, Collections.emptyMap());
         }
 
         public DateFieldType(String name, DateFormatter dateFormatter) {
             this(name, true, false, true, dateFormatter, Resolution.MILLISECONDS, null, Collections.emptyMap());
         }
 
         public DateFieldType(String name, Resolution resolution) {
-            this(name, true, false, true, DEFAULT_DATE_TIME_FORMATTER, resolution, null, Collections.emptyMap());
+            this(name, true, false, true, getDefaultDateTimeFormatter(), resolution, null, Collections.emptyMap());
         }
 
         public DateFieldType(String name, Resolution resolution, DateFormatter dateFormatter) {

server/src/main/java/org/opensearch/index/mapper/RangeFieldMapper.java

@@ -95,7 +95,7 @@ public class RangeFieldMapper extends ParametrizedFieldMapper {
      */
     public static class Defaults {
         public static final Explicit<Boolean> COERCE = new Explicit<>(true, false);
-        public static final DateFormatter DATE_FORMATTER = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER;
+        public static final DateFormatter DATE_FORMATTER = DateFieldMapper.getDefaultDateTimeFormatter();
     }
 
     // this is private since it has a different default

server/src/main/java/org/opensearch/index/mapper/RangeType.java

@@ -313,7 +313,7 @@ public Query rangeQuery(
         ) {
             ZoneId zone = (timeZone == null) ? ZoneOffset.UTC : timeZone;
 
-            DateMathParser dateMathParser = (parser == null) ? DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.toDateMathParser() : parser;
+            DateMathParser dateMathParser = (parser == null) ? DateFieldMapper.getDefaultDateTimeFormatter().toDateMathParser() : parser;
             boolean roundUp = includeLower == false; // using "gt" should round lower bound up
             Long low = lowerTerm == null
                 ? minValue()

server/src/main/java/org/opensearch/index/mapper/RootObjectMapper.java

@@ -72,7 +72,7 @@ public class RootObjectMapper extends ObjectMapper {
      */
     public static class Defaults {
         public static final DateFormatter[] DYNAMIC_DATE_TIME_FORMATTERS = new DateFormatter[] {
-            DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER,
+            DateFieldMapper.getDefaultDateTimeFormatter(),
             DateFormatter.forPattern("yyyy/MM/dd HH:mm:ss||yyyy/MM/dd||epoch_millis") };
         public static final boolean DATE_DETECTION = true;
         public static final boolean NUMERIC_DETECTION = false;

server/src/main/java/org/opensearch/script/ScoreScriptUtils.java

@@ -343,11 +343,14 @@ public double decayNumericGauss(double docValue) {
     /**
      * Limitations: since script functions don't have access to DateFieldMapper,
      * decay functions on dates are limited to dates in the default format and default time zone,
+     * Further, since script module gets initialized before the featureflags are loaded,
+     * we cannot use the feature flag to gate the usage of the new default date format.
      * Also, using calculations with <code>now</code> are not allowed.
      *
      */
     private static final ZoneId defaultZoneId = ZoneId.of("UTC");
-    private static final DateMathParser dateParser = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.toDateMathParser();
+    // ToDo: use new default date formatter once feature flag is removed
+    private static final DateMathParser dateParser = DateFieldMapper.LEGACY_DEFAULT_DATE_TIME_FORMATTER.toDateMathParser();
 
     /**
      * Linear date decay

server/src/main/java/org/opensearch/search/aggregations/support/CoreValuesSourceType.java

@@ -411,7 +411,7 @@ public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing,
         @Override
         public DocValueFormat getFormatter(String format, ZoneId tz) {
             return new DocValueFormat.DateTime(
-                format == null ? DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER : DateFormatter.forPattern(format),
+                format == null ? DateFieldMapper.getDefaultDateTimeFormatter() : DateFormatter.forPattern(format),
                 tz == null ? ZoneOffset.UTC : tz,
                 // If we were just looking at fields, we could read the resolution from the field settings, but we need to deal with script
                 // output, which has no way to indicate the resolution, so we need to default to something. Milliseconds is the standard.

server/src/main/java/org/opensearch/search/aggregations/support/ValueType.java

@@ -61,7 +61,7 @@ public enum ValueType implements Writeable {
         "date",
         "date",
         CoreValuesSourceType.DATE,
-        new DocValueFormat.DateTime(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER, ZoneOffset.UTC, DateFieldMapper.Resolution.MILLISECONDS)
+        new DocValueFormat.DateTime(DateFieldMapper.getDefaultDateTimeFormatter(), ZoneOffset.UTC, DateFieldMapper.Resolution.MILLISECONDS)
     ),
     IP((byte) 6, "ip", "ip", CoreValuesSourceType.IP, DocValueFormat.IP),
     // TODO: what is the difference between "number" and "numeric"?

server/src/test/java/org/opensearch/action/search/BottomSortValuesCollectorTests.java

@@ -46,7 +46,6 @@
 import java.time.ZoneId;
 import java.util.Arrays;
 
-import static org.opensearch.index.mapper.DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.apache.lucene.search.TotalHits.Relation.GREATER_THAN_OR_EQUAL_TO;
@@ -136,7 +135,11 @@ public void testWithDates() {
         for (boolean reverse : new boolean[] { true, false }) {
             SortField[] sortFields = new SortField[] { new SortField("foo", SortField.Type.LONG, reverse) };
             DocValueFormat[] sortFormats = new DocValueFormat[] {
-                new DocValueFormat.DateTime(DEFAULT_DATE_TIME_FORMATTER, ZoneId.of("UTC"), DateFieldMapper.Resolution.MILLISECONDS) };
+                new DocValueFormat.DateTime(
+                    DateFieldMapper.getDefaultDateTimeFormatter(),
+                    ZoneId.of("UTC"),
+                    DateFieldMapper.Resolution.MILLISECONDS
+                ) };
             BottomSortValuesCollector collector = new BottomSortValuesCollector(3, sortFields);
             collector.consumeTopDocs(
                 createTopDocs(sortFields[0], 100, newDateArray("2017-06-01T12:18:20Z", "2018-04-03T15:10:27Z", "2013-06-01T13:10:20Z")),
@@ -170,7 +173,11 @@ public void testWithDateNanos() {
         for (boolean reverse : new boolean[] { true, false }) {
             SortField[] sortFields = new SortField[] { new SortField("foo", SortField.Type.LONG, reverse) };
             DocValueFormat[] sortFormats = new DocValueFormat[] {
-                new DocValueFormat.DateTime(DEFAULT_DATE_TIME_FORMATTER, ZoneId.of("UTC"), DateFieldMapper.Resolution.NANOSECONDS) };
+                new DocValueFormat.DateTime(
+                    DateFieldMapper.getDefaultDateTimeFormatter(),
+                    ZoneId.of("UTC"),
+                    DateFieldMapper.Resolution.NANOSECONDS
+                ) };
             BottomSortValuesCollector collector = new BottomSortValuesCollector(3, sortFields);
             collector.consumeTopDocs(
                 createTopDocs(sortFields[0], 100, newDateNanoArray("2017-06-01T12:18:20Z", "2018-04-03T15:10:27Z", "2013-06-01T13:10:20Z")),
@@ -242,15 +249,15 @@ private Object[] newBytesArray(String... values) {
     private Object[] newDateArray(String... values) {
         Long[] longs = new Long[values.length];
         for (int i = 0; i < values.length; i++) {
-            longs[i] = DEFAULT_DATE_TIME_FORMATTER.parseMillis(values[i]);
+            longs[i] = DateFieldMapper.getDefaultDateTimeFormatter().parseMillis(values[i]);
         }
         return longs;
     }
 
     private Object[] newDateNanoArray(String... values) {
         Long[] longs = new Long[values.length];
         for (int i = 0; i < values.length; i++) {
-            longs[i] = DateUtils.toNanoSeconds(DEFAULT_DATE_TIME_FORMATTER.parseMillis(values[i]));
+            longs[i] = DateUtils.toNanoSeconds(DateFieldMapper.getDefaultDateTimeFormatter().parseMillis(values[i]));
         }
         return longs;
     }