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

Improve ElasticSearchGraphServiceTest syncAfterWrite #3124

Closed
EnricoMi opened this issue Aug 19, 2021 · 3 comments · Fixed by #3160 · May be fixed by #10408
Closed

Improve ElasticSearchGraphServiceTest syncAfterWrite #3124

EnricoMi opened this issue Aug 19, 2021 · 3 comments · Fixed by #3160 · May be fixed by #10408
Labels
feature-request Request for a new feature to be added

Comments

@EnricoMi
Copy link
Contributor

All tests in ElasticSearchGraphServiceTest depend on syncAfterWrite to wait for modifications done to the graph to become visible. The current implementation waits 5 seconds, which is not ideal as it does not guarantee success and unnecessarily slows down the tests.

A better aproach would be to wait until ElasticSearch notifies that outstanding modifications have been committed. This would significantly improve speed. Currently all tests take 10 minutes, where 20 seconds would be possible.

The following approach does not work:

@Override
protected void syncAfterWrite() throws Exception {
  _searchClient.indices().flush(new FlushRequest(), RequestOptions.DEFAULT);
}

There are many tests in #3011 that fail without any sync wait.
@EnricoMi EnricoMi added the feature-request Request for a new feature to be added label Aug 19, 2021
@gabe-lyons
Copy link
Contributor

@EnricoMi do you have a syncAfterWrite implementation that does work? We tested a number of different ways to sync after writing but were unable to find one that worked with the mock elastic service.

@EnricoMi EnricoMi mentioned this issue Aug 25, 2021
Merged
@EnricoMi
Copy link
Contributor Author

@gabe-lyons looks like flush is not enough, we also need a RefreshRequest:

_searchClient.indices().refresh(new RefreshRequest(), RequestOptions.DEFAULT);

Then, tests are very quick.

Fixed in #3160.

@EnricoMi
Copy link
Contributor Author

EnricoMi commented Sep 8, 2021 •
edited

The flush and refresh are not sufficient for some tests, so I have also added another sync approach. It adds data to ElasticSearch and busy-waits for it to appear. Then it removes that data and busy-waits for that to disappear. The idea is that this guarantees data added earlier becomes visible by then. See 86d9c61.

Unfortunately, a single test is still flaky. Test testConcurrentAddEdge fails in 5% of the runs not seeing data that has been added, where 95% of the runs the test passes. Until other ways of guaranteeing visibility of writes are found, this test is skipped. See b14d03a.

@trialiya trialiya mentioned this issue May 1, 2024
Open
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature-request Request for a new feature to be added
Projects
None yet
Milestone
No milestone
2 participants
@gabe-lyons @EnricoMi