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.

Sign up for GitHub

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

Jump to bottom

make DateMathIndexExpressionsIntegrationIT more resilient #38473

Merged
merged 2 commits into from
Feb 8, 2019
Merged

make DateMathIndexExpressionsIntegrationIT more resilient #38473

merged 2 commits into from
Feb 8, 2019

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Feb 5, 2019

the internal cluster calls System.nanoTime() and System.currentTimeMillis() during
evaluations of requests that need date-math index resolution. These are not mockable
in these tests. As is, executing requests as-is in these test cases can potentially result
in invalid responses when day-boundaries are hit mid test run.

This change makes the test framework ignore failures due to day changes.

Closes #31067.

@talevy talevy added >test Issues or PRs that are addressing/adding tests :Core/Infra/Core Core issues without another label v7.0.0 labels Feb 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@talevy talevy added :Data Management/Indices APIs APIs to create and manage indices and templates and removed :Core/Infra/Core Core issues without another label labels Feb 5, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@talevy
Copy link
Contributor Author

talevy commented Feb 5, 2019

happy to collect any feedback anyone has, I tried recreating the issue
locally by constantly increasing the time on my machine during the test run.
Wasn't sure how else to control time since it is set in two separate places and
not injectable.

@talevy
make DateMathIndexExpressionsIntegrationIT more resilient
79f209a 
the internal cluster calls System.nanoTime() and System.currentTimeMillis() during evaluations of requests
that need date-math index resolution. These are not mockable in these tests. As is, executing requests as-is
in these test cases can potentially result in invalid responses when day-boundaries are hit mid test run.

This change makes the test framework ignore failures due to day changes.

Closes elastic#31067.
@talevy talevy requested a review from pgomulka February 5, 2019 23:20
@talevy
Copy link
Contributor Author

talevy commented Feb 6, 2019

run elasticsearch-ci/1

@pgomulka
Copy link
Contributor

pgomulka commented Feb 6, 2019

in some places (watcher plugin integration tests HistoryIntegrationTests ) the ClockMock is used. Ideally it would be nice if we could pass some random values there the same way we pass seed and datetime. Not sure if that is doable..

@jasontedor jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019
@pgomulka pgomulka mentioned this pull request Feb 6, 2019
Merged
@talevy
Copy link
Contributor Author

talevy commented Feb 6, 2019

that is a tough one. IndexNameExpressionResolver#concreteIndexNames and IndexNameExpressionResolver#concreteIndices are called directly by a lot of transport actions to resolves index names, and a Clock instance is not used. To resolve this would require refactoring of 35-40 transport classes. I thought this would be a quicker way to get a win for a bug that can only occur once in a day if this test is run at the exact moment a day changes.

@talevy
Copy link
Contributor Author

talevy commented Feb 6, 2019

I guess this is no rush, so maybe the refactoring route is a better one

@cbuescher
Copy link
Member

cbuescher commented Feb 6, 2019 •
edited
Loading

@talevy just cross-linking this issue that I just opened about some time-related test failures in watcher tests (HistoryIntegrationTests) just in case you want to add some thoughts about "controlling time" in these kind of situations. Maybe it doesn't apply but might be worth adding a few ideas if you thought about something in the context of this PR.

pgomulka
pgomulka reviewed Feb 7, 2019
View reviewed changes
server/src/test/java/org/elasticsearch/indices/DateMathIndexExpressionsIntegrationIT.java
response = builder.get();
} catch (IndexNotFoundException e) {
// index resolver throws this if it does not find the exact index due to day changes
dayChangeAssumption.run();
Copy link
Contributor

@pgomulka pgomulka Feb 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed previous comment. I see it is working. Getdayofyear will change..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason this is here is because the System.nanotime's day that is picked up in a search request's start-time can differ from the indices that were created, which will cause indices to be resolved to ones that do not exist. This means that the dayChange occurred between the time of the first dayChangeAssumption.run(), and the time the search had begun. Although this time window is super small, could still happen and happened to me once during testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when this occurs, I set a dayChangeAssumption check to run before throwing the exception so that if the error is because of the day change, I want the test to be ignored. If there was an IndexNotFoundException thrown and the day didn't change, then there is a real problem and the exception should be re-thrown.

does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you for making it clear. I think this is a good change and we should have this merged before we make an attempt to refactor this in a more general way

pgomulka
pgomulka approved these changes Feb 8, 2019
View reviewed changes
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given our build is getting flakey it is definitely worthy to have this merged
thank you for looking into this!
LGTM

@pgomulka
Copy link
Contributor

pgomulka commented Feb 8, 2019

@elasticmachine run elasticsearch-ci/1

@talevy
Copy link
Contributor Author

talevy commented Feb 8, 2019

thanks @pgomulka!

@talevy talevy merged commit a1c63a3 into elastic:master Feb 8, 2019
@talevy talevy deleted the fix-31067 branch February 8, 2019 21:09
jasontedor added a commit to liketic/elasticsearch that referenced this pull request Feb 10, 2019
@jasontedor
Merge branch 'master' into pr/28895
e3508d1 
* master: (1159 commits)
  Fix timezone fallback in ingest processor (elastic#38407)
  Avoid polluting download stats on builds (elastic#38660)
  SQL: Prevent grouping over grouping functions (elastic#38649)
  SQL: Relax StackOverflow circuit breaker for constants (elastic#38572)
  [DOCS] Fixes broken migration links (elastic#38655)
  Drop support for the low-level REST client on JDK 7 (elastic#38540)
  [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641)
  fix dissect doc "ip" --> "clientip" (elastic#38545)
  Concurrent file chunk fetching for CCR restore (elastic#38495)
  make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473)
  SQL: Replace joda with java time (elastic#38437)
  Add fuzziness example (elastic#37194) (elastic#38648)
  Mute AnalysisModuleTests#testStandardFilterBWC (elastic#38636)
  add geotile_grid ref to asciidoc (elastic#38632)
  Enable Dockerfile from artifacts.elastic.co (elastic#38552)
  Mute FollowerFailOverIT testFailOverOnFollower (elastic#38634)
  Account for a possible rolled over file while reading the audit log file (elastic#34909)
  Mute failure in InternalEngineTests (elastic#38622)
  Fix Issue with Concurrent Snapshot Init + Delete (elastic#38518)
  Refactor ZonedDateTime.now in millis resolution (elastic#38577)
  ...
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Feb 11, 2019
@jasontedor
Merge branch 'master' into retention-lease-ccr
baec3a0 
* master:
  Fix timezone fallback in ingest processor (elastic#38407)
  Avoid polluting download stats on builds (elastic#38660)
  SQL: Prevent grouping over grouping functions (elastic#38649)
  SQL: Relax StackOverflow circuit breaker for constants (elastic#38572)
  [DOCS] Fixes broken migration links (elastic#38655)
  Drop support for the low-level REST client on JDK 7 (elastic#38540)
  [DOCS] Adds placeholders for v8 highlights, breaking changes, release notes (elastic#38641)
  fix dissect doc "ip" --> "clientip" (elastic#38545)
  Concurrent file chunk fetching for CCR restore (elastic#38495)
  make DateMathIndexExpressionsIntegrationIT more resilient (elastic#38473)
  SQL: Replace joda with java time (elastic#38437)
  Add fuzziness example (elastic#37194) (elastic#38648)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pgomulka pgomulka pgomulka approved these changes

Assignees
No one assigned
Labels
:Data Management/Indices APIs APIs to create and manage indices and templates >test Issues or PRs that are addressing/adding tests v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DateMathIndexExpressionsIntegrationIT#testIndexNameDateMathExpressions() fails in CI
6 participants
@talevy @elasticmachine @pgomulka @cbuescher @jakelandis @jasontedor