Fix DateMathExpressionResolverTests tests #37037 #37059

jsoref · 2018-12-31T22:15:57Z

https://www.juandebravo.com/2015/04/10/java-yyyy-date-format/

In short:
YYYY is "4 digit year for use with week number notation"
yyyy is "4 digit year for use with month day notation"

#37037

jsoref · 2018-12-31T22:17:34Z

I lost quite a few hours looking into this. My first commit wasn't a complete fix, there were other instances of YYYY w/ date formatters.

Thanks to freenode #java for the pointer

jsoref · 2018-12-31T22:42:09Z

@javanna

elasticmachine · 2019-01-02T09:14:02Z

Pinging @elastic/es-core-infra

javanna · 2019-01-02T09:14:17Z

test this please

javanna · 2019-01-02T09:14:36Z

@rjernst would you have a chance to look into this one and the related test failures?

server/src/test/java/org/elasticsearch/cluster/metadata/DateMathExpressionResolverTests.java

jsoref · 2019-01-02T21:27:46Z

@javanna, can you ask the bot to retest?

javanna · 2019-01-03T09:09:34Z

@jsoref could you also check out and unmute RolloverIT#testRolloverWithDateMath please ?

droberts195

I left a few notes about the file structure finder files. Please revert the changes to (only) the file structure finder files in this PR. I know I need to do work on this for 6.7 but it's not quite the same as the other changes in this PR and I would rather separate it out.

docs/reference/ml/apis/find-file-structure.asciidoc

...n/ml/src/main/java/org/elasticsearch/xpack/ml/filestructurefinder/TimestampFormatFinder.java

https://www.juandebravo.com/2015/04/10/java-yyyy-date-format/ In short for the Java SimpleDateFormat class: YYYY is "4 digit year for use with week number notation" yyyy is "4 digit year for use with month day notation" See https://docs.oracle.com/javase/7/docs/api/java/text/SimpleDateFormat.html Roughly, cases with YYYY/YY + W/WW/WWW should be using YYYY/YY, and pretty much everything else wants yyyy/yy. Notably: Joda's DateTimeFormat class is different: https://www.joda.org/joda-time/apidocs/org/joda/time/format/DateTimeFormat.html

jsoref · 2019-01-03T14:35:22Z

@droberts195: done

droberts195 · 2019-01-03T14:48:20Z

Thanks @jsoref.

Next time please push a new commit rather than amending an existing one that someone has commented on, because amending the commit that has comments stops others viewing the full context around the comments. But I can see you've removed all the files related to the file structure finder so don't worry about it this time.

I no longer have an objection to this PR being merged once @rjernst and @javanna are happy.

concerns addressed

javanna · 2019-01-03T19:29:21Z

retest this please

javanna · 2019-01-04T09:01:41Z

run gradle build tests 2

spinscale · 2019-01-07T10:16:34Z

One of the core issues of the DateMathExpressionResolverTests is to use a Context that uses System.currentTimeInMillis(). One should use a context uses a randomized time for proper reproduction.

+    // some date between 1970 and 2040
+    private final long currentTime = randomLongBetween(0, 2240521200000L);
     private final Context context = new Context(
-            ClusterState.builder(new ClusterName("_name")).build(), IndicesOptions.strictExpand()
+        ClusterState.builder(new ClusterName("_name")).build(), IndicesOptions.strictExpand(), currentTime
     );

jsoref · 2019-01-08T21:02:44Z

@javanna is this me?

javanna · 2019-01-08T21:17:01Z

is this me?

@jsoref I am not sure :) You mean if you should make the change that @spinscale suggested?

jsoref · 2019-01-08T21:24:12Z

Well, both the last build failure (default-distro) and also that change...

I'm really just trying to make a small incremental change. It shouldn't really be blocked on every possible thing in your codebase... It sounds like @spinscale understands the change -- to me it seems better for that to be authored by @spinscale than by me...

javanna · 2019-01-08T21:27:53Z

I agree @jsoref it sounds like the suggested change can be made as a follow-up. I think the build bit that was red is not your fault, I need to re-run it, let me find the magic command to run only that part... ;)

javanna · 2019-01-08T21:28:52Z

run sample packaging tests

javanna · 2019-01-08T21:29:11Z

ops, wrong one :)

javanna · 2019-01-08T21:34:05Z

run default distro tests

droberts195 · 2019-01-14T12:14:18Z

run default distro tests

droberts195 · 2019-01-14T12:16:04Z

FYI the changes to the structure finder that I said I would do in #37059 (review) are in #37306.

javanna · 2019-01-29T14:22:16Z

Is this something that we would like to get in? Is it worth addressing merge conflicts or? Please can you comment @rjernst , @spinscale or re-assign?

droberts195 · 2019-02-04T09:50:17Z

Jenkins test this please

droberts195 · 2019-02-04T09:54:52Z

I think it would be good to get this merged into master before 7.0 and 7.x are branched.

On 31st December 2018 a whole load of CI builds failed due to YYYY meaning week year in Java time. If we don't merge this then this year the same tests will fail on both 30th and 31st December (due to the way week years diverge from "normal" calendar years on 0-3 days).

Also, it's setting a bad example to people who copy the formats from the code if we're still using YYYY in formats that are going to be used by the Java time parser.

I merged latest master into the PR branch to try to progress this.

But I think approval to merge should come from someone who is working on the Java time project.

spinscale

there are a few changes that look like they happened by search & replace, that should be reverted

libs/grok/src/main/resources/patterns/grok-patterns

server/src/main/java/org/joda/time/format/StrictISODateTimeFormat.java

server/src/test/java/org/elasticsearch/common/time/DateFormattersTests.java

spinscale · 2019-02-04T12:37:42Z

x-pack/docs/en/watcher/release-notes.asciidoc

@@ -201,7 +201,7 @@ October 28, 2015
  they did before the upgrade. For example if `watcher.dynamic_indices.time_zone`
  setting was set to `+01:00` and a watch has the following index name
  `<logstash-{now/d}>` then after the upgrade you need to update this watch to
-  use the following index name `<logstash-{now/d{YYYY.MM.dd|+01:00}}>`.
+  use the following index name `<logstash-{now/d{yyyy.MM.dd|+01:00}}>`.


these are old release notes, that might be outdated in other ways as well, I am not sure if it is worth updating?

spinscale · 2019-02-04T12:38:24Z

server/src/test/java/org/elasticsearch/script/JodaCompatibleZonedDateTimeTests.java

@@ -232,8 +232,8 @@ public void testYearOfEra() {
    }

    public void testToString1() {
-        assertMethodDeprecation(() -> assertThat(javaTime.toString("YYYY/MM/dd HH:mm:ss.SSS"),
-            equalTo(jodaTime.toString("YYYY/MM/dd HH:mm:ss.SSS"))), "toString(String)", "a DateTimeFormatter");
+        assertMethodDeprecation(() -> assertThat(javaTime.toString("yyyy/MM/dd HH:mm:ss.SSS"),


I am confused by reading this. Shouldnt one be in capital letters and one not in order for this to work?

Java time cares significantly about Y vs y. Afaict, Joda time only uses Y vs y to exclude 0 and negative numbers (actually, the documentation I can find differs about whether or not 0 is a valid value, but...).

That said, y in joda is the "year" w/o the constraint, which is a closer map to Java y.

...lugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/history/HistoryStoreField.java

spinscale · 2019-02-04T12:40:53Z

...ck/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporter.java

@@ -70,7 +70,7 @@
            Setting.affixKeySetting("xpack.monitoring.exporters.","index.name.time_format",
                    key -> Setting.simpleString(key, Property.Dynamic, Property.NodeScope));

-    private static final String INDEX_FORMAT = "YYYY.MM.dd";
+    private static final String INDEX_FORMAT = "yyyy.MM.dd";


the monitoring side of things is fixed in #35809

danielmitterdorfer · 2019-03-08T10:07:24Z

Thank you for your PR but it seems that this has been fixed meanwhile in #38376. I hope that you understand that I therefore close this PR unmerged.

jsoref · 2019-03-08T16:49:40Z

@danielmitterdorfer: I understand in some ways. At the end of the day, I don't care who gets credit for fixing things -- I just want the thing fixed so it doesn't interfere with whatever I'm actually trying to accomplish.

From a process perspective, I'm disappointed in the way it was managed. Ideally if someone wants to adopt a ticket the previous person working on it should be informed so they can stop worrying about it/working on it. There's clearly enough information in GitHub for this to happen -- both PRs were tied to the same issue. I'm glad to be informed, but a month delay in being informed seems a bit long. I hope you understand that perspective.

javanna added the :Core/Infra/Core Core issues without another label label Jan 2, 2019

javanna added the >bug label Jan 2, 2019

javanna requested a review from rjernst January 2, 2019 09:14

javanna reviewed Jan 2, 2019

View reviewed changes

server/src/test/java/org/elasticsearch/cluster/metadata/DateMathExpressionResolverTests.java Outdated Show resolved Hide resolved

jsoref force-pushed the year-37037 branch from f0c8032 to 61f80f8 Compare January 2, 2019 15:53

droberts195 previously requested changes Jan 3, 2019

View reviewed changes

jsoref force-pushed the year-37037 branch from 40cce07 to 0e0a88a Compare January 3, 2019 14:31

droberts195 approved these changes Jan 3, 2019

View reviewed changes

droberts195 mentioned this pull request Jan 30, 2019

Make date formats in filebeat compatible with Joda and Java time formats elastic/beats#10433

Closed

7 tasks

Merge branch 'master' into year-37037

a54e40b

spinscale suggested changes Feb 4, 2019

View reviewed changes

Revert items per spinscale

0ff919b

spinscale mentioned this pull request Feb 18, 2019

Tests: Use date based year instead of week based one #37033

Closed

danielmitterdorfer closed this Mar 8, 2019

jsoref deleted the year-37037 branch March 8, 2019 16:41

Fix DateMathExpressionResolverTests tests #37037 #37059

Fix DateMathExpressionResolverTests tests #37037 #37059

Uh oh!

Conversation

jsoref commented Dec 31, 2018

Uh oh!

jsoref commented Dec 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsoref commented Dec 31, 2018

Uh oh!

elasticmachine commented Jan 2, 2019

Uh oh!

javanna commented Jan 2, 2019

Uh oh!

javanna commented Jan 2, 2019

Uh oh!

Uh oh!

jsoref commented Jan 2, 2019

Uh oh!

javanna commented Jan 3, 2019

Uh oh!

droberts195 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsoref commented Jan 3, 2019

Uh oh!

droberts195 commented Jan 3, 2019

Uh oh!

javanna commented Jan 3, 2019

Uh oh!

javanna commented Jan 4, 2019

Uh oh!

spinscale commented Jan 7, 2019

Uh oh!

jsoref commented Jan 8, 2019

Uh oh!

javanna commented Jan 8, 2019

Uh oh!

jsoref commented Jan 8, 2019

Uh oh!

javanna commented Jan 8, 2019

Uh oh!

javanna commented Jan 8, 2019

Uh oh!

javanna commented Jan 8, 2019

Uh oh!

javanna commented Jan 8, 2019

Uh oh!

droberts195 commented Jan 14, 2019

Uh oh!

droberts195 commented Jan 14, 2019

Uh oh!

javanna commented Jan 29, 2019

Uh oh!

droberts195 commented Feb 4, 2019

Uh oh!

droberts195 commented Feb 4, 2019

Uh oh!

spinscale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spinscale Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

spinscale Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

jsoref Feb 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spinscale Feb 4, 2019

Choose a reason for hiding this comment

jsoref commented Dec 31, 2018 •

edited

Loading