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

Delete Operations on Replicas when Segment Replication is enabled #2839

Merged
merged 8 commits into from
Apr 13, 2022
Merged

Delete Operations on Replicas when Segment Replication is enabled #2839

merged 8 commits into from
Apr 13, 2022

Conversation

Rishikesh1159
Copy link
Member

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

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

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.

@Rishikesh1159 Rishikesh1159 requested a review from a team as a code owner April 11, 2022 15:20
@Rishikesh1159 Rishikesh1159 requested a review from mch2 April 11, 2022 15:21
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 1670ba1
Log 4350

Reports 4350

@Rishikesh1159 Rishikesh1159 changed the title Feature/segment replication Delete Operations on Replicas when Segment Replication is enabled Apr 11, 2022
mch2
mch2 previously approved these changes Apr 11, 2022
Copy link
Member

@mch2 mch2 left a comment

Choose a reason for hiding this comment

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

LGTM few nits.


createIndex(
INDEX_NAME,
Settings.builder()
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed this on the last PR. You shouldn't need to set these settings here because we override the indexSettings() method at the top of this file.

        createIndex(INDEX_NAME);

Will create the index with the default settings we override.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah sorry I too missed it

@@ -716,11 +717,13 @@ 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.
// for segment replication: replicas should create the reader from store and, we don't want an open IW on replicas.
/* We should always wrap replicas with a SoftDeletesDirectoryReaderWrapper as we use soft deletes when segment replication is on for
Copy link
Member

Choose a reason for hiding this comment

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

nit - "We should always wrap the DirectoryReader used on replicas with SoftDeletesDirectoryReaderWrapper so that we filter out soft deletes.

}
return DirectoryReader.open(indexWriter);
return DirectoryReader.open(indexWriter, true, true);
Copy link
Member

@mch2 mch2 Apr 11, 2022

Choose a reason for hiding this comment

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

is this required? does your test pass without this like with the first PR?

I forgot this also impacts the reader with segrep off - if this isn't required with segrep on then we should remove it. Otherwise it needs to be gated by reading the setting. It is changing how the primary behaves with segrep off.

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't required with segrep on. I think we remove it

@mch2 mch2 dismissed their stale review April 11, 2022 22:08

changes required.

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure abced8c
Log 4414

Reports 4414

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 9da9d29
Log 4415

Reports 4415

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success f9cf43e
Log 4417

Reports 4417

@Rishikesh1159 Rishikesh1159 requested a review from mch2 April 12, 2022 21:24
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 5a0f68b
Log 4425

Reports 4425

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9948fa0
Log 4430

Reports 4430

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success e7e3c31
Log 4431

Reports 4431

@Rishikesh1159 Rishikesh1159 merged commit 61ecbc8 into opensearch-project:feature/segment-replication Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants