-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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 date-processor to a new default year for every new pipeline execution #22601
Conversation
@@ -105,14 +106,28 @@ public void testJodaPatternLocale() { | |||
} | |||
|
|||
public void testJodaPatternDefaultYear() { | |||
// fix joda's new DateTime() to Jan 12, 2017 | |||
DateTimeUtils.setCurrentMillisFixed(1484255848828L); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be the way Joda set up to do such mock-hackery... I don't especially like it since this may affect other threads running during testing (not sure).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one minor comment on the test
assertThat(ingestDocument.getFieldValue("date_as_date", String.class), equalTo("2018-06-12T00:00:00.000+02:00")); | ||
|
||
// cleanup | ||
DateTimeUtils.setCurrentMillisSystem(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be done in a finally
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds like a plan. I thought about it but realized the test is already somewhat making assumptions that things will go right, but I'll wrap it for extra measure 👍
…tion. Beforehand, the DateProcessor constructs its joda pattern formatter during processor construction. This led to newly ingested documents being defaulted to the year that the pipeline was constructed, not that of processing. Fixes elastic#22547.
@rjernst I've reverted the tests to pass security checks |
LGTM |
retest this please |
2 similar comments
retest this please |
retest this please |
* master: (47 commits) Remove non needed import use expectThrows instead of manually testing exception Fix checkstyle and a test Update after review Read ec2 discovery address from aws instance tags Invalidate cached query results if query timed out (elastic#22807) Add remaining generated painless API Generate reference links for painless API (elastic#22775) [TEST] Fix ElasticsearchExceptionTests Add parsing method for ElasticsearchException.generateThrowableXContent() (elastic#22783) Improve connection closing in `RemoteClusterConnection` (elastic#22804) Docs: Cluster allocation explain should be on one page Remove DFS_QUERY_AND_FETCH as a search type (elastic#22787) Add repository-url module and move URLRepository (elastic#22752) fix date-processor to a new default year for every new pipeline execution. (elastic#22601) Add tests for top_hits aggregation (elastic#22754) [TEST] Added this for 93a28b0 submitted via elastic#22772 Fix typo in comment in OsProbe.java Add new ruby search library to community clients doc (elastic#22765) RangeQuery WITHIN case now normalises query (elastic#22431) ...
Beforehand, the DateProcessor constructs its joda pattern formatter during processor
construction. This led to newly ingested documents being defaulted to
the year that the pipeline was constructed, not that of processing.
Fixes #22547.