From 0513d8dca3155bd0ef881c8c84133f913f7a2d0e Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Tue, 21 Jan 2020 09:02:21 +0100 Subject: [PATCH] Add SPI jvm option to SystemJvmOptions (#50916) Adding back accidentally removed jvm option that is required to enforce start of the week = Monday in IsoCalendarDataProvider. Adding a `feature` to yml test in order to skip running it in JDK8 commit that removed it 398c802 commit that backports SystemJvmOptions c4fbda3 relates 7.x backport of code that enforces CalendarDataProvider use #48349 --- .../tools/launchers/SystemJvmOptions.java | 27 +++++++++--- .../310_date_agg_per_day_of_week.yml | 44 +++++++++++++++++++ .../common/time/IsoCalendarDataProvider.java | 7 +++ .../joda/JavaJodaTimeDuellingTests.java | 9 ++++ .../test/rest/yaml/Features.java | 16 +++++-- 5 files changed, 95 insertions(+), 8 deletions(-) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/310_date_agg_per_day_of_week.yml diff --git a/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/SystemJvmOptions.java b/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/SystemJvmOptions.java index 80903a040c0..f4b080489ac 100644 --- a/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/SystemJvmOptions.java +++ b/distribution/tools/launchers/src/main/java/org/elasticsearch/tools/launchers/SystemJvmOptions.java @@ -19,6 +19,8 @@ package org.elasticsearch.tools.launchers; +import org.elasticsearch.tools.java_version_checker.JavaVersion; + import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -60,11 +62,26 @@ final class SystemJvmOptions { // log4j 2 "-Dlog4j.shutdownHookEnabled=false", "-Dlog4j2.disable.jmx=true", - /* - * Due to internationalization enhancements in JDK 9 Elasticsearch need to set the provider to COMPAT otherwise time/date - * parsing will break in an incompatible way for some date patterns and locales. - */ - "-Djava.locale.providers=COMPAT")); + + javaLocaleProviders())); + } + + private static String javaLocaleProviders() { + /** + * SPI setting is used to allow loading custom CalendarDataProvider + * in jdk8 it has to be loaded from jre/lib/ext, + * in jdk9+ it is already within ES project and on a classpath + * + * Due to internationalization enhancements in JDK 9 Elasticsearch need to set the provider to COMPAT otherwise time/date + * parsing will break in an incompatible way for some date patterns and locales. + * //TODO COMPAT will be deprecated in jdk14 https://bugs.openjdk.java.net/browse/JDK-8232906 + * See also: documentation in server/org.elasticsearch.common.time.IsoCalendarDataProvider + */ + if(JavaVersion.majorVersion(JavaVersion.CURRENT) == 8){ + return "-Djava.locale.providers=SPI,JRE"; + } else { + return "-Djava.locale.providers=SPI,COMPAT"; + } } } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/310_date_agg_per_day_of_week.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/310_date_agg_per_day_of_week.yml new file mode 100644 index 00000000000..8a69638f9cc --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/310_date_agg_per_day_of_week.yml @@ -0,0 +1,44 @@ + +setup: + - skip: + version: " - 7.5.99" + reason: "Start of the week Monday was backported to 7.6" + features: "spi_on_classpath_jdk9" + + - do: + indices.create: + index: test + body: + mappings: + properties: + date: + type: date + - do: + index: + index: test + id: 1 + body: { "date": "2009-11-15T14:12:12" } + - do: + indices.refresh: + index: [test] +--- +# The inserted document has a field date=2009-11-15T14:12:12 which is Sunday. +# When aggregating per day of the week this should be considered as last day of the week (7) +# and this value should be used in 'key_as_string' +"Date aggregation per day of week": + - do: + search: + rest_total_hits_as_int: true + index: test + body: + aggregations: + test: + "date_histogram": { + "field": "date", + "calendar_interval": "day", + "format": "e", + "offset": 0 + } + - match: {hits.total: 1} + - length: { aggregations.test.buckets: 1 } + - match: { aggregations.test.buckets.0.key_as_string: "7" } diff --git a/server/src/main/java/org/elasticsearch/common/time/IsoCalendarDataProvider.java b/server/src/main/java/org/elasticsearch/common/time/IsoCalendarDataProvider.java index 36a93049ade..e066109567e 100644 --- a/server/src/main/java/org/elasticsearch/common/time/IsoCalendarDataProvider.java +++ b/server/src/main/java/org/elasticsearch/common/time/IsoCalendarDataProvider.java @@ -22,6 +22,13 @@ import java.util.Calendar; import java.util.Locale; import java.util.spi.CalendarDataProvider; +/** + * This class is loaded by JVM SPI mechanism in order to provide ISO compatible behaviour for week calculations using java.time. + * It defines start of the week as Monday and requires 4 days in the first week of the year. + * This class overrides default behaviour for Locale.ROOT (start of the week Sunday, minimum 1 day in the first week of the year). + * Java SPI mechanism requires java.locale.providers to contain SPI value (i.e. java.locale.providers=SPI,COMPAT) + * @see ISO week date + */ public class IsoCalendarDataProvider extends CalendarDataProvider { @Override diff --git a/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java b/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java index 7e3ed3abcb3..7825577d010 100644 --- a/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java +++ b/server/src/test/java/org/elasticsearch/common/joda/JavaJodaTimeDuellingTests.java @@ -29,6 +29,7 @@ import org.joda.time.DateTime; import org.joda.time.DateTimeZone; import org.joda.time.format.DateTimeFormat; import org.joda.time.format.ISODateTimeFormat; +import org.junit.BeforeClass; import java.time.ZoneId; import java.time.ZoneOffset; @@ -47,6 +48,14 @@ public class JavaJodaTimeDuellingTests extends ESTestCase { return false; } + @BeforeClass + public static void checkJvmProperties(){ + boolean runtimeJdk8 = JavaVersion.current().getVersion().get(0) == 8; + assert (runtimeJdk8 && ("SPI,JRE".equals(System.getProperty("java.locale.providers")))) + || (false == runtimeJdk8 && ("SPI,COMPAT".equals(System.getProperty("java.locale.providers")))) + : "`-Djava.locale.providers` needs to be set"; + } + public void testTimezoneParsing() { /** this testcase won't work in joda. See comment in {@link #testPartialTimeParsing()} * assertSameDateAs("2016-11-30T+01", "strict_date_optional_time", "strict_date_optional_time"); diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java index bdcf426d118..83104229ea3 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java @@ -19,11 +19,12 @@ package org.elasticsearch.test.rest.yaml; +import org.elasticsearch.bootstrap.JavaVersion; +import org.elasticsearch.test.rest.ESRestTestCase; + import java.util.Arrays; import java.util.List; -import org.elasticsearch.test.rest.ESRestTestCase; - import static java.util.Collections.unmodifiableList; /** @@ -50,6 +51,7 @@ public final class Features { "transform_and_set", "arbitrary_key" )); + private static final String SPI_ON_CLASSPATH_SINCE_JDK_9 = "spi_on_classpath_jdk9"; private Features() { @@ -68,10 +70,18 @@ public final class Features { if (ESRestTestCase.hasXPack()) { return false; } - } else if (false == SUPPORTED.contains(feature)) { + } else if (false == isSupported(feature)) { return false; } } return true; } + + private static boolean isSupported(String feature) { + if(feature.equals(SPI_ON_CLASSPATH_SINCE_JDK_9) && + JavaVersion.current().compareTo(JavaVersion.parse("9")) >= 0) { + return true; + } + return SUPPORTED.contains(feature) ; + } }