-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 issue of red index on close for remote enabled clusters #15990
base: main
Are you sure you want to change the base?
Conversation
❌ Gradle check result for a1d5a87: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
I have added tests for the edge case that is mentioned in the referenced issue. I had added the tests first and then the main code changes -
|
❌ Gradle check result for ac864f0: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
ac864f0
to
29cf87f
Compare
❌ Gradle check result for 29cf87f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Fixed this. |
❕ Gradle check result for 4420487: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15990 +/- ##
============================================
- Coverage 71.92% 71.90% -0.03%
+ Complexity 64400 64384 -16
============================================
Files 5281 5281
Lines 300995 301000 +5
Branches 43479 43481 +2
============================================
- Hits 216491 216428 -63
- Misses 66793 66816 +23
- Partials 17711 17756 +45 ☔ View full report in Codecov by Sentry. |
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.
Wondering is this the problem with just remote translog or local translog as well?
The problem seems to exist for remote translog only since the local version seems fine. When we close the index (in case of remote translog), the translog is wiped out locally first and the rehydrated from remote store. At the point, the most recent checkpoint file downloaded has a global checkpoint from the last but 1 translog sync. |
4420487
to
89789f4
Compare
Signed-off-by: Ashish Singh <[email protected]>
89789f4
to
27b6828
Compare
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.
Changes looks great .
latch.await(); | ||
// Sleep for some time for the next doc to be present in lucene buffer. If flush happens first before the doc #2 | ||
// gets indexed, then it goes into the happy case where the close index happens succefully. | ||
Thread.sleep(1000); |
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.
curious why do we need sleep here ? As we have already indexed docs and they should be in lucene buffer.
ensureGreen(INDEX_NAME); | ||
} | ||
|
||
public void testCloseIndexWithNoOpSyncAndFlushForAsyncTranslog() throws InterruptedException { |
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.
can we write UTs as well for same ?
Description
The close index operation involves following steps -
During a happy index close, we upload translog twice -
However, if there is a flush that has happened after the operation landed in the Lucene buffer but before the buffered sync (for sync translog) or the periodic async sync (for async translog), then the steps 3(a) and 3(b) becomes no-op and the GCP uploaded in the checkpoint file would be the one from the last translog sync. This causes the discrepancy between maxSeqNo and GCP and causing exception while creating ReadOnlyEngine leading to red index.
In this PR, changes are made to track the global checkpoint that has been updated as part of the successful translog upload to remote store. The new tracked global checkpoint is now also used in the
RemoteFsTranslog.syncNeeded()
method and checked against the current (translog writer) last synced global checkpoint.Related Issues
Resolves #15989
Check List
[ ] API changes companion pull request created, if applicable.[ ] Public documentation issue/PR created, if applicable.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.