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

Don't dump a stacktrace for invalid patterns when executing elasticse… #49744

Merged
merged 1 commit into from Jan 2, 2020

Conversation

gaobinlong
Copy link
Contributor

The PR is related to #49642.
When executing elasticsearch-croneval tool, it throws an exception and prints the stacktrace if the cron pattern is not valid. The change in this PR is catching the exception and printing the detail message only.

Before this change, when executing elasticsearch-croneval with invalid cron patterns:
./bin/elasticsearch-croneval "*"
it prints:
Exception in thread "main" java.lang.IllegalArgumentException: invalid cron expression [*] at org.elasticsearch.xpack.core.watcher.support.Exceptions.illegalArgument(Exceptions.java:22) at org.elasticsearch.xpack.core.scheduler.Cron.<init>(Cron.java:275) at org.elasticsearch.xpack.core.scheduler.Cron.validate(Cron.java:792)
After this change, it prints:

ERROR: invalid cron expression [illegal cron expression format [java.lang.IllegalArgumentException: unexpected end of expression at pos [1].]]

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (:Core/Features/Watcher)

@dakrone dakrone self-assigned this Dec 5, 2019
@dakrone
Copy link
Member

dakrone commented Dec 18, 2019

@gaobinlong thanks for opening this! We discussed it and have some feedback. Would you be able to add an additional command line argument that will still allow someone to access the full stacktrace if needed? It would be off by default, but in place in the event more diagnostic information is needed.

@gaobinlong
Copy link
Contributor Author

@dakrone OK, I will do that.

@gaobinlong
Copy link
Contributor Author

@dakrone I have added -d(--detail) argument for elasticsearch-croneval, which can be used to print the stacktrace if the cron expression is not valid. Can you help to review the change? Thanks!

@dakrone dakrone self-requested a review January 2, 2020 16:58
Copy link
Member

@dakrone dakrone 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 iterating on this @gaobinlong, I'll merge this soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants