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

Set start of the week to Monday for root locale using SPI #45384

Closed
wants to merge 15 commits into from

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Aug 9, 2019

Set start of the week to Monday for root locale (#43652) …
Introducing a IsoLocal.ROOT constant which should be used instead of java.util.Locale.ROOT in ES when dealing with dates. IsoLocal.ROOT customises start of the week to be Monday instead of Sunday.

closes #42588 an issue with investigation details
relates #41670 bug raised (this won't fix it on its own. joda.parseInto has to be reimplemented
closes #43275 an issue raised by community member

Introducing a IsoLocal.ROOT constant which should be used instead of java.util.Locale.ROOT in ES when dealing with dates. IsoLocal.ROOT  customises start of the week to be Monday instead of Sunday.

closes elastic#42588 an issue with investigation details
relates elastic#41670 bug raised (this won't fix it on its own. joda.parseInto has to be reimplemented
closes elastic#43275 an issue raised by community member
@pgomulka pgomulka added :Core/Infra/Core Core issues without another label v7.4.0 labels Aug 9, 2019
@pgomulka pgomulka self-assigned this Aug 9, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@pgomulka pgomulka changed the title Set start of the week to Monday for root locale backport(#43652) Set start of the week to Monday for root locale using SPI Aug 12, 2019
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

One question

@@ -39,6 +40,28 @@
import static org.hamcrest.Matchers.is;

public class JavaJodaTimeDuellingTests extends ESTestCase {
@Override
protected boolean enableWarningsCheck() {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to disable warning checks? Can't we use assertWarnings if necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will have a look to enable this again

@@ -111,9 +111,11 @@ ${error.file}
8:-XX:+UseGCLogFileRotation
8:-XX:NumberOfGCLogFiles=32
8:-XX:GCLogFileSize=64m
# due to CalendarDataProvider being customized to ISO standard, SPI has to be first provider to try.
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.

Can you relocate this so it's co-located with the similar JDK 9 argument?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@pgomulka
Copy link
Contributor Author

the SPI mechanism works fine for jdks 9+. However it won't work for jdk8 in the same way. Before jdk9 only classes loaded with java extension mechanism could be used as SPI for packages java.util.*. The class we want to load with SPI is java.util.spi.CalendarDataProvider

So for jdk8 we would have to either place the jar with the SPI class in the lib/ext or made sure that we use java.ext.dirs

The JDK 9 release notes mentioning this change Release Note: Locale sensitive SPI and Input Method SPI implementations are now loaded from application's classpath

The bug raised against jdk9 to fix this Support java.util.spi., java.text.spi., java.awt.im.spi loaded from classpath

SPI loading in jdk8 https://github.com/bpupadhyaya/openjdk-8/blob/master/jdk/src/share/classes/sun/util/locale/provider/SPILocaleProviderAdapter.java#L80
SPI loading in jdk9 https://github.com/AdoptOpenJDK/openjdk-jdk9/blob/master/jdk/src/java.base/share/classes/sun/util/locale/provider/SPILocaleProviderAdapter.java#L81
Note different classloaders

@colings86 colings86 added v7.5.0 and removed v7.4.0 labels Aug 30, 2019
@pgomulka
Copy link
Contributor Author

closed in favour of #48349

@pgomulka pgomulka closed this Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport :Core/Infra/Core Core issues without another label v7.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants