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 date-processor to a new default year for every new pipeline execution #22601

Merged
merged 1 commit into from
Jan 25, 2017

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Jan 12, 2017

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.

@talevy talevy added :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >bug v5.3.0 v6.0.0-alpha1 labels Jan 12, 2017
@talevy talevy requested a review from rjernst January 12, 2017 21:31
@@ -105,14 +106,28 @@ public void testJodaPatternLocale() {
}

public void testJodaPatternDefaultYear() {
// fix joda's new DateTime() to Jan 12, 2017
DateTimeUtils.setCurrentMillisFixed(1484255848828L);
Copy link
Contributor Author

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).

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, 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();
Copy link
Member

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?

Copy link
Contributor Author

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.
@talevy
Copy link
Contributor Author

talevy commented Jan 23, 2017

@rjernst I've reverted the tests to pass security checks

@rjernst
Copy link
Member

rjernst commented Jan 23, 2017

LGTM

@talevy
Copy link
Contributor Author

talevy commented Jan 24, 2017

retest this please

2 similar comments
@talevy
Copy link
Contributor Author

talevy commented Jan 25, 2017

retest this please

@talevy
Copy link
Contributor Author

talevy commented Jan 25, 2017

retest this please

@talevy talevy merged commit e9a68b3 into elastic:master Jan 25, 2017
@talevy talevy deleted the fix-22547 branch January 25, 2017 23:09
talevy added a commit that referenced this pull request Jan 25, 2017
…tion. (#22601)

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.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Jan 26, 2017
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP v5.3.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants