Delete Operations on Replicas when Segment Replication is enabled #2804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mch2
requested changes
Apr 7, 2022
| @@ -718,7 +704,7 @@ private ExternalReaderManager createReaderManager(RefreshWarmerListener external | |||
| private DirectoryReader getDirectoryReader() throws IOException { | |||
| // for segment replication: replicas should create the reader from store, we don't want an open IW on replicas. | |||
There was a problem hiding this comment.
Lets update this comment with a bit more on why we need to wrap our reader with a SoftDeletesDirectoryReaderWrapper.
| @@ -718,7 +704,7 @@ private ExternalReaderManager createReaderManager(RefreshWarmerListener external | |||
| private DirectoryReader getDirectoryReader() throws IOException { | |||
| // for segment replication: replicas should create the reader from store, we don't want an open IW on replicas. | |||
| if (engineConfig.isReadOnly()) { | |||
| return DirectoryReader.open(store.directory()); | |||
| return new SoftDeletesDirectoryReaderWrapper(DirectoryReader.open(store.directory()), Lucene.SOFT_DELETES_FIELD); | |||
| } | |||
| return DirectoryReader.open(indexWriter); | |||
There was a problem hiding this comment.
do we not need writeAllDeletes set to true here? DirectoryReader.open(indexWriter, true, true);
| @@ -95,10 +92,18 @@ protected OpenSearchDirectoryReader refreshIfNeeded(OpenSearchDirectoryReader re | |||
| // If not using NRT repl. | |||
| if (currentInfos == null) { | |||
| reader = (OpenSearchDirectoryReader) DirectoryReader.openIfChanged(referenceToRefresh); | |||
| if (reader != null) { | |||
| logger.info("Num docs primary {}", reader.getDelegate().numDocs()); | |||
There was a problem hiding this comment.
these logger.infos look left over from testing?
| @@ -2286,6 +2295,9 @@ public SegmentInfos getLatestSegmentInfos() { | |||
| OpenSearchDirectoryReader reader = null; | |||
| try { | |||
| reader = externalReaderManager.internalReaderManager.acquire(); | |||
| if (engineConfig.isReadOnly()) { | |||
| return ((StandardDirectoryReader)((SoftDeletesDirectoryReaderWrapper) reader.getDelegate()).getDelegate()).getSegmentInfos(); | |||
There was a problem hiding this comment.
Can you please add a comment here that this is safe because we always wrap the Standard reader with a SoftDeletesDirectoryReaderWrapper for replicas when segrep is on. This will be more clear once we have a separate engine class.
| @@ -229,4 +229,45 @@ public void onFailure(Exception e) { | |||
| } | |||
| }); | |||
| } | |||
|
|
|||
| public void testDelOps()throws Exception{ | |||
There was a problem hiding this comment.
nit - no need to abbreviate. testDeleteOperations.
Signed-off-by: Rishikesh1159 <[email protected]>
Signed-off-by: Rishikesh1159 <[email protected]>
Signed-off-by: Rishikesh1159 <[email protected]>
This reverts commit 24e3769. Signed-off-by: Rishikesh1159 <[email protected]>
* Fix Segment replication integ tests by using a single BackgroundIndexer. BackgroundIndexers begin indexing docs with a docId of 0 up to the requested numDocs. Using two overlapped the docIds so counts were incorrect. This changes our tests to use a single indexer, it also improves assertions on segment data on both shards. Signed-off-by: Marc Handalian <[email protected]> * Fix Index shards to correctly compare checkpoint version instead of segment gen when determining if a received checkpoint should be processed. Without this change replicas will ignore checkpoints for merges. Signed-off-by: Marc Handalian <[email protected]> * Updated based on PR feedback. Rename latestUnprocessedCheckpoint to latestReceivedCheckpoint. Remove redundant checkpoint variable in onNewCheckpoint. Rename isValidCheckpoint to shouldProcessCheckpoint. Add missing isAheadOf check to shouldProcessCheckpoint. Update SegmentReplicationIT's assertSegmentStats to be more readable. Removed latest seqNo hack. Signed-off-by: Marc Handalian <[email protected]> Signed-off-by: Rishikesh1159 <[email protected]>
Signed-off-by: Rishikesh1159 <[email protected]>
Signed-off-by: Rishikesh1159 <[email protected]>
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes delete operations to only write to xlog on replicas. Integration Test for this is also added
Issues Resolved
#2330
Check List
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.