Skip to content
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

Add SPI jvm option to SystemJvmOptions #50916

Merged
merged 15 commits into from Jan 21, 2020

Conversation

@pgomulka
Copy link
Contributor

pgomulka commented Jan 13, 2020

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

Adding back accidently removed jvm options that is required to enfoce
start of the week = Monday in IsoCalendarDataProvider.
@pgomulka pgomulka self-assigned this Jan 13, 2020
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

elasticmachine commented Jan 13, 2020

Pinging @elastic/es-core-infra (:Core/Infra/Core)

# 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":

This comment has been minimized.

Copy link
@pugnascotia

pugnascotia Jan 13, 2020

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

This comment has been minimized.

Copy link
@pgomulka

pgomulka Jan 13, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@pgomulka

pgomulka Jan 15, 2020

Author Contributor

@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

This comment has been minimized.

Copy link
@pugnascotia

pugnascotia Jan 16, 2020

Contributor

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?

This comment has been minimized.

Copy link
@pgomulka

pgomulka Jan 17, 2020

Author Contributor

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

@pugnascotia pugnascotia dismissed their stale review Jan 13, 2020

I realised that CI is still failing

Copy link
Member

rjernst left a comment

A couple comments


# 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

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 14, 2020

Member

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

This comment has been minimized.

Copy link
@pgomulka

pgomulka Jan 15, 2020

Author Contributor

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

* 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"));

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 14, 2020

Member

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

This comment has been minimized.

Copy link
@pgomulka

pgomulka Jan 15, 2020

Author Contributor

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

pgomulka and others added 6 commits Jan 15, 2020
…sticsearch into fix/spi_provider_jvm_option
@pgomulka

This comment has been minimized.

Copy link
Contributor Author

pgomulka commented Jan 15, 2020

@elasticmachine run elasticsearch-ci/2

pgomulka and others added 2 commits Jan 15, 2020
…regation/310_date_agg_per_day_of_week.yml

Co-Authored-By: Rory Hunter <pugnascotia@users.noreply.github.com>
@polyfractal polyfractal added v7.7.0 and removed v7.6.0 labels Jan 15, 2020
@pgomulka

This comment has been minimized.

Copy link
Contributor Author

pgomulka commented Jan 15, 2020

@elasticmachine run elasticsearch-ci/2

pgomulka added 4 commits Jan 18, 2020
@pgomulka

This comment has been minimized.

Copy link
Contributor Author

pgomulka commented Jan 20, 2020

@elasticmachine run elasticsearch-ci/2

@pgomulka pgomulka merged commit 0513d8d into elastic:7.x Jan 21, 2020
9 checks passed
9 checks passed
CLA All commits in pull request signed
Details
elasticsearch-ci/1 Build finished.
Details
elasticsearch-ci/2 Build finished.
Details
elasticsearch-ci/bwc Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docs Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample-matrix-unix Build finished.
Details
elasticsearch-ci/packaging-sample-matrix-windows Build finished.
Details
pgomulka added a commit that referenced this pull request Jan 21, 2020
The test should be run against 7.7 version at least, as this was only backported and released in that version

relates SPI based implementation #48209
relates backport #50916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.