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

Add Delete Operations to Translog from Engine #2980

Closed
wants to merge 2 commits into from
Closed

Add Delete Operations to Translog from Engine #2980

wants to merge 2 commits into from

Conversation

Rishikesh1159
Copy link
Member

Signed-off-by: Rishikesh1159 rishireddy1159@gmail.com

Description

This PR will allow us to decouple adding Delete operations to Lucene and adding Delete operations to Translog. This would be useful for segment replication, as we will need to just add delete operation to replicas translog instead of performing full delete operation. We will only use this method when segment replication is enabled and when we are on a replica node. The logic for checking if segment replication is enabled and is replica node will be added later in a different PR.

This is a part of the process of merging our feature branch - feature/segment-replication - back into main by re-PRing our changes from the feature branch.
The breakdown of the plan to merge to main is detailed here: #2926
For added context on segment replication - here's the design proposal #2229

Issues Resolved

#2926

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.

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
@Rishikesh1159 Rishikesh1159 requested review from a team and reta as code owners April 19, 2022 17:35
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 9f655ff
Log 4623

Reports 4623

@kartg
Copy link
Member

kartg commented Apr 19, 2022

Same note as #2977 (comment) - please add javadocs for the new API

Other than that, LGTM!

Signed-off-by: Rishikesh1159 <rishireddy1159@gmail.com>
@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure a7ea737
Log 4670

Reports 4670

@Rishikesh1159
Copy link
Member Author

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success a7ea737
Log 4671

Reports 4671

@kartg
Copy link
Member

kartg commented Apr 21, 2022

Note - we're holding off on pushing this as per #2977 (comment)

Copy link
Collaborator

@nknize nknize left a comment

Choose a reason for hiding this comment

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

I figured this is implicit by now but wanted to explicitly mark this to be merged when we have a PR that uses the method.

@kotwanikunal
Copy link
Member

@Rishikesh1159 Any updates on this? Are you still blocked?

@Rishikesh1159
Copy link
Member Author

Closing this PR. Changes necessary are added in a different PR.

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.

5 participants