[Watcher] Improved error messages for CronEvalTool by lipsill · Pull Request #32800 · elastic/elasticsearch

lipsill · 2018-08-12T10:38:18Z

CronEvalTool prints an error only for cron expressions that result in
no coming time events.

If a cron expression results in less than the specified -count
(default 10) time events, the CronEvalTool prints all the coming
times and displays no error message.

Fixes #32735

CronEvalTool prints an error only for cron expressions that result in no coming time events. If a cron expression results in less than the specified `-count` (default 10) time events, the CronEvalTool prints all the coming times and displays no error message. Fixes elastic#32735

elasticmachine · 2018-08-13T10:57:30Z

Pinging @elastic/es-core-infra

spinscale

Thanks a bunch for tackling this! I left some comments to simplify the code and make the tests more readable.

Would be awesome if you could take a further look, but I am happy to take over if you do not want to!

spinscale · 2018-08-14T12:27:05Z

...atcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java

-            terminal.println((i+1) + ".\t" + formatter.print(time));
+            times.add((i + 1) + ".\t" + formatter.print(time));
        }
+        terminal.println((i == count ? "Here are the next " : "There are ") + i + " times this cron expression will trigger:");


I don't think we need this special handling here (introducing state that requires following by filling up the list). Just adding the additional if-statement plus break should be sufficient.

I made the change as originally the following results were printed by the cli tool:
0 0 0 1-6 * ? resulted in

Valid! Now is [Wed, 15 Aug 2018 11:28:34] Here are the next 10 times this cron expression will trigger: 1. Sat, 1 Sep 2018 02:00:00 2. Sun, 2 Sep 2018 02:00:00 3. Mon, 3 Sep 2018 02:00:00 4. Tue, 4 Sep 2018 02:00:00 5. Wed, 5 Sep 2018 02:00:00 6. Thu, 6 Sep 2018 02:00:00 7. Mon, 1 Oct 2018 02:00:00 8. Tue, 2 Oct 2018 02:00:00 9. Wed, 3 Oct 2018 02:00:00 10. Thu, 4 Oct 2018 02:00:00

And a one time cron, such as 0 3 23 8 9 ? 2019 resulted in

Valid! Now is [Wed, 15 Aug 2018 11:29:37] Here are the next 10 times this cron expression will trigger: 1. Mon, 9 Sep 2019 01:03:00

I found it is a little bit confusing that in this case the message was Here are the next 10 time and only a single one was printed, hence I tried to clarify the message.

I will revert the change.

spinscale · 2018-08-14T12:27:35Z

...r/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java

    }
+
+    public void testGetNextValidTimes() throws Exception {
+        {


for readability, just put final int year = ZonedDateTime.now(ZoneOffset.UTC).getYear() + 1; at the top of the test and use that one maybe?

spinscale · 2018-08-14T12:28:28Z

...r/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java

+        {
+            String output = execute(
+                    "0 3 23 8 9 ? " + (Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1));
+            assertTrue(output, output.contains("There are 1 times this cron expression will trigger:"));


can you replace assertTrue/assertFalse with

assertThat(output, containsString("FoObar")); assertThat(output, not(containsString("ERROR")));

This way the assertion will return something helpful instead of just being true or false in order to ease debugging.

lipsill · 2018-08-15T11:47:57Z

@spinscale thanks for the feedback! I just pushed the changes. Can you have another look?

spinscale · 2018-08-16T08:27:47Z

@elasticmachine test this please

spinscale

left two super minor things I'd like to fix, but apart from that it's good. let's see what CI has to say!

spinscale · 2018-08-16T08:28:47Z

...atcher/src/main/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalTool.java

 import org.joda.time.format.DateTimeFormatter;

+import java.util.Arrays;
+import java.util.List;


in order to keep the diff small, can you keep the imports as is?

spinscale · 2018-08-16T08:29:26Z

...r/src/test/java/org/elasticsearch/xpack/watcher/trigger/schedule/tool/CronEvalToolTests.java

+
+    public void testGetNextValidTimes() throws Exception {
+        {
+            final int nextYear = Calendar.getInstance(TimeZone.getTimeZone("UTC"), Locale.ROOT).get(Calendar.YEAR) + 1;


for readability, just put final int year = ZonedDateTime.now(ZoneOffset.UTC).getYear() + 1; at the top of the test and use that one maybe?

lipsill · 2018-08-16T11:06:48Z

@spinscale I hope I addressed your latest comments.
Thanks for kicking a complete CI ;)

spinscale · 2018-08-16T11:25:37Z

@elasticmachine test this please

lipsill · 2018-08-16T13:05:57Z

Hm... the second CI run finished with an error that I cannot reproduce locally. I actually doubt that the test failure is caused by the changes in this PR.
@spinscale can you have a look when you have a chance? Thanks!

spinscale · 2018-08-20T07:12:09Z

@elasticmachine retest this please

spinscale · 2018-08-20T07:12:40Z

once CI passes, I'll merge this. Thanks a lot for helping!

spinscale · 2018-08-20T10:17:15Z

@elasticmachine retest this please

spinscale · 2018-08-21T15:03:10Z

may I ask you to merge the master branch into yours one more time, so we can trigger CI and see if it passes? Thanks a ton!

lipsill · 2018-08-22T10:34:01Z

@spinscale done! Let's see if this helps ;)

spinscale · 2018-08-22T12:08:32Z

@elasticmachine retest this please

CronEvalTool prints an error only for cron expressions that result in no upcoming time events. If a cron expression results in less than the specified count (default 10) time events, now all the coming times are printed without displaying error message. Closes #32735

spinscale · 2018-08-23T08:37:07Z

thanks again, I have merged the PR into the master and 6.x branch

* es/master: (62 commits) [DOCS] Add docs for Application Privileges (#32635) Add versions 5.6.12 and 6.4.1 Do NOT allow termvectors on nested fields (#32728) [Rollup] Return empty response when aggs are missing (#32796) [TEST] Add some ACL yaml tests for Rollup (#33035) Move non duplicated actions back into xpack core (#32952) Test fix - GraphExploreResponseTests should not randomise array elements Closes #33086 Use `addIfAbsent` instead of checking if an element is contained TESTS: Fix Random Fail in MockTcpTransportTests (#33061) HLRC: Fix Compile Error From Missing Throws (#33083) [DOCS] Remove reload password from docs cf. #32889 HLRC: Add ML Get Buckets API (#33056) Watcher: Improve error messages for CronEvalTool (#32800) Search: Support of wildcard on docvalue_fields (#32980) Change query field expansion (#33020) INGEST: Cleanup Redundant Put Method (#33034) SQL: skip uppercasing/lowercasing function tests for AZ locales as well (#32910) Fix the default pom file name (#33063) Switch ml basic tests to new style Requests (#32483) Switch some watcher tests to new style Requests (#33044) ...

* es/6.x: (58 commits) [DOCS] Add docs for Application Privileges (#32635) Add versions 5.6.12 and 6.4.1 [Rollup] Return empty response when aggs are missing (#32796) [TEST] Add some ACL yaml tests for Rollup (#33035) Test fix - GraphExploreResponseTests should not randomise array elements Closes #33086 Use `addIfAbsent` instead of checking if an element is contained HLRC: Fix Compile Error From Missing Throws (#33083) [DOCS] Remove reload password from docs cf. #32889 Use a dedicated ConnectionManger for RemoteClusterConnection (#32988) Watcher: Improve error messages for CronEvalTool (#32800) HLRC: Add ML Get Buckets API (#33056) Change query field expansion (#33020) Search: Support of wildcard on docvalue_fields (#32980) Add beta label to MSI on install Elasticsearch page (#28126) SQL: skip uppercasing/lowercasing function tests for AZ locales as well (#32910) [DOCS] Drafts Elasticsearch 6.4.0 release notes (#33039) Fix the default pom file name (#33063) Fix backport of switch ml basic tests to new style Requests (#32483) Switch ml basic tests to new style Requests (#32483) Switch some watcher tests to new style Requests (#33044) ...

lipsill · 2018-08-28T14:29:43Z

Thanks @spinscale :)

colings86 added the :Distributed/Watcher label Aug 13, 2018

colings86 added the >enhancement label Aug 13, 2018

spinscale requested changes Aug 14, 2018

View reviewed changes

spinscale self-assigned this Aug 14, 2018

lipsill added 2 commits August 15, 2018 13:41

addressing reviewers comments

a784510

replace assertTrue by assertThat

40d86d8

remove leftover import

161c0d4

spinscale requested changes Aug 16, 2018

View reviewed changes

some minor cleanup

51dcb90

spinscale approved these changes Aug 20, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into CronEvalTool_errorMsg

5bbadcf

spinscale merged commit f84ed14 into elastic:master Aug 23, 2018

spinscale added the v7.0.0 label Aug 23, 2018

spinscale added the v6.5.0 label Aug 23, 2018

lipsill deleted the CronEvalTool_errorMsg branch October 24, 2018 11:41

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

lipsill commented Aug 12, 2018

Uh oh!

elasticmachine commented Aug 13, 2018

Uh oh!

spinscale left a comment

Choose a reason for hiding this comment

Uh oh!

spinscale Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

lipsill Aug 15, 2018

Choose a reason for hiding this comment

Uh oh!

spinscale Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

spinscale Aug 14, 2018

Choose a reason for hiding this comment

Uh oh!

lipsill commented Aug 15, 2018

Uh oh!

spinscale commented Aug 16, 2018

Uh oh!

spinscale left a comment

Choose a reason for hiding this comment

Uh oh!

spinscale Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

spinscale Aug 16, 2018

Choose a reason for hiding this comment

Uh oh!

lipsill commented Aug 16, 2018

Uh oh!

spinscale commented Aug 16, 2018

Uh oh!

lipsill commented Aug 16, 2018

Uh oh!

spinscale commented Aug 20, 2018

Uh oh!

spinscale commented Aug 20, 2018

Uh oh!

spinscale commented Aug 20, 2018

Uh oh!

spinscale commented Aug 21, 2018

Uh oh!

lipsill commented Aug 22, 2018

Uh oh!

spinscale commented Aug 22, 2018

Uh oh!

spinscale commented Aug 23, 2018

Uh oh!

lipsill commented Aug 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants