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

Fix java time epoch date formatters #37829

Merged
merged 12 commits into from
Feb 1, 2019

Conversation

spinscale
Copy link
Contributor

The self written epoch date formatters were not properly able to format
an Instant to a string due to a misconfiguration.

This fix also removes a until now existing runtime behaviour under java
8 regarding the names of the aggregation buckets, which are now the same
as before and have been under java 11.

The self written epoch date formatters were not properly able to format
an Instant to a string due to a misconfiguration.

This fix also removes a until now existing runtime behaviour under java
8 regarding the names of the aggregation buckets, which are now the same
as before and have been under java 11.
@spinscale spinscale added >non-issue :Core/Infra/Core Core issues without another label v7.0.0 labels Jan 24, 2019
@spinscale spinscale requested a review from rjernst January 24, 2019 16:07
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@@ -72,7 +70,7 @@ public TemporalAccessor resolve(Map<TemporalField,Long> fieldValues,
private static final EpochField NANOS_OF_SECOND = new EpochField(ChronoUnit.NANOS, ChronoUnit.SECONDS, ValueRange.of(0, 999_999_999)) {
@Override
public boolean isSupportedBy(TemporalAccessor temporal) {
return temporal.isSupported(ChronoField.NANO_OF_SECOND) && temporal.getLong(ChronoField.NANO_OF_SECOND) != 0;
return temporal.isSupported(ChronoField.NANO_OF_SECOND);
Copy link
Member

Choose a reason for hiding this comment

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

I added this condition to guard from printing out ".0" when nanos are 0. Why is this being removed? This test passes currently but fails with this PR:

DateFormatter secondsFormatter = DateFormatter.forPattern("epoch_second");
Instant instant = Instant.ofEpochSecond(42, 0);
assertThat(secondsFormatter.format(instant), equalTo("42"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm, this one passed for me on java8 and java11 in this PR. Any test setup that I am missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the trick is, that you can have an optional fractional with a length of 0

@spinscale
Copy link
Contributor Author

@elasticmachine retest elasticsearch-ci/1

1 similar comment
@spinscale
Copy link
Contributor Author

@elasticmachine retest elasticsearch-ci/1

@spinscale
Copy link
Contributor Author

this PR seems to consistently crash JVMs on our test infra. I was not yet lucky to reproduce this locally on my linux bare metal machine...

@spinscale
Copy link
Contributor Author

@rjernst can you take another look at this one please? You mentioned your test fails in this PR, can you help me to reproduce if that is still the case?

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.

LGTM. Thanks for figuring out the trick about zero length fraction! The test I ran before passes now.

@spinscale spinscale merged commit c02cd3e into elastic:master Feb 1, 2019
spinscale added a commit to spinscale/elasticsearch that referenced this pull request Feb 1, 2019
The self written epoch date formatters were not properly able to format
an Instant to a string due to a misconfiguration.

This also adds some tests for fractional formatters
spinscale added a commit that referenced this pull request Feb 1, 2019
The self written epoch date formatters were not properly able to format
an Instant to a string due to a misconfiguration.

This also adds some tests for fractional formatters
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 1, 2019
* master: (36 commits)
  Ensure joda compatibility in custom date formats (elastic#38171)
  Do not compute cardinality if the `terms` execution mode does not use `global_ordinals` (elastic#38169)
  Do not set timeout for IndexRequests in GatewayIndexStateIT (elastic#38147)
  Zen2ify testMasterFailoverDuringIndexingWithMappingChanges (elastic#38178)
  SQL: [Docs] Add limitation for aggregate functions on scalars (elastic#38186)
  Add elasticsearch-node detach-cluster command (elastic#37979)
  Add tests for fractional epoch parsing (elastic#38162)
  Enable bw tests for elastic#37871 and elastic#38032. (elastic#38167)
  Clear send behavior rule in CloseWhileRelocatingShardsIT (elastic#38159)
  Fix testCorruptedIndex (elastic#38161)
  Add finalReduce flag to SearchRequest (elastic#38104)
  Forbid negative field boosts in analyzed queries (elastic#37930)
  Remove AtomiFieldData#getLegacyFieldValues (elastic#38087)
  Universal cluster bootstrap method for tests with autoMinMasterNodes=false (elastic#38038)
  Fix FullClusterRestartIT.testHistoryUUIDIsAdded (elastic#38098)
  Replace joda time in ingest-common module (elastic#38088)
  Fix eclipse config for ssl-config (elastic#38096)
  Don't load global ordinals with the `map` execution_hint (elastic#37833)
  Relax fault detector in some disruption tests (elastic#38101)
  Fix java time epoch date formatters (elastic#37829)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants