Skip to content

Add SPI jvm option to SystemJvmOptions #50916

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Jan 21, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions distribution/src/config/jvm.options
Original file line number Diff line number Diff line change
Expand Up @@ -75,3 +75,8 @@ ${error.file}

# JDK 9+ GC logging
9-:-Xlog:gc*,gc+age=trace,safepoint:file=${loggc}:utctime,pid,tags:filecount=32,filesize=64m

# 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 locals
8-:-Djava.locale.providers=SPI,JRE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this just be 8, not 8-?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, this should just be 8 - removed in favour of conditional check in SystemJvmOptions

9-:-Djava.locale.providers=SPI,COMPAT
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,7 @@ static List<String> systemJvmOptions() {
"-Dio.netty.allocator.numDirectArenas=0",
// 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"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we keep this in system jvm options? Shouldn't this be something a user doesn't need to think about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are right, it is better to stay in SystemJvmOptions. We can always check the version running with JavaVersion.majorVersion(JavaVersion.CURRENT) == 8 to choose the right settings

"-Dlog4j2.disable.jmx=true"));
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@

setup:
- skip:
version: " - 7.5.99" #todo this test failing on jdk8. has to be removed from 7.x
reason: "Start of the week Monday was backported to 7.5"


- 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 aggregartion per day of week":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Date aggregartion per day of week":
"Date aggregation per day of week":

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - as per todo this test is expected to fail in 1.8 https://github.com/elastic/elasticsearch/pull/50916/files#diff-a1dab819a2bedbcb781ecd85580cfbb5R4
I am actually not sure how to work around this, the code for using IsoCalendarDataProvider is in 7.x but because we support JDK1.8 I won't be able to always pass the build.
Any suggestions how to fix this?
I am thinking of just removing the test for now..

Copy link
Contributor Author

@pgomulka pgomulka Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pugnascotia @rjernst what is your view on this? Maybe we should extend the testing framework to allow checking runtime jvm and skip basing on this?
how about we work around this with features ? see the latest changes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My feeling is that these changes are fine, but what I'm missing is some Javadoc to explain why they're necessary. Unless I have all the context around dates and JDK versions, I can't understand what the checks are for. The new feature name doesn't help here either. Can we add some more explanation into the code, to help other developers understand what is going on and why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pugnascotia good idea. will add some more javadocs as they are clearly missing

- 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" }
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
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;
Expand All @@ -47,6 +48,12 @@ protected boolean enableWarningsCheck() {
return false;
}

@BeforeClass
public static void checkJvmProperties(){
assert ("SPI,COMPAT".equals(System.getProperty("java.locale.providers")))
: "`-Djava.locale.providers=SPI,COMPAT` 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");
Expand Down