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

[Backport 2.x] [Segment Replication] Add a new Engine implementation for replicas with segment replication enabled. #4003

Conversation

Rishikesh1159
Copy link
Member

Signed-off-by: Rishikesh1159 [email protected]

Description

This PR backports PR to 2.x branch and resolve conflict in LocalCheckpointTrackerTests, LocalCheckpointTracker classes.

Issues Resolved

#3109

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.

…tion for newChangesSnapshotFromTranslogFile() in NRTReplicationEngine

Signed-off-by: Rishikesh1159 <[email protected]>
@Rishikesh1159 Rishikesh1159 requested review from a team and reta as code owners July 25, 2022 18:50

@Deprecated
@Override
public Translog.Snapshot newChangesSnapshotFromTranslogFile(String source, long fromSeqNo, long toSeqNo, boolean requiredFullRange)
Copy link
Member Author

Choose a reason for hiding this comment

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

@mch2 I added this implementation temporarily, let me know if this is good or you just want to return null or entirely remove this method from Engine as it will be deprecated

Copy link
Member

Choose a reason for hiding this comment

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

Removing the method is a breaking change. We can't introduce that in a backport. I would suggest we use the same impl as InternalEngine here.

    public Translog.Snapshot newChangesSnapshotFromTranslogFile(String source, long fromSeqNo, long toSeqNo, boolean requiredFullRange)
        throws IOException {
        return getTranslog().newSnapshot(fromSeqNo, toSeqNo, requiredFullRange);
    }

Comment on lines 172 to 177
/**
* Return the latest active SegmentInfos from the engine.
* @return {@link SegmentInfos}
*/
protected abstract SegmentInfos getLatestSegmentInfos();

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this a breaking change for 2.x?

Copy link
Member

Choose a reason for hiding this comment

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

Good call @Rishikesh1159 We'll need to provide default impl here and remove abstract.

@mch2
Copy link
Member

mch2 commented Jul 25, 2022

We will also need to backport changes to NRTReplicationEngine introduced with translog manager - #3671 and #3751

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #4003 (d28778e) into 2.x (fb4f96f) will decrease coverage by 0.09%.
The diff coverage is 68.45%.

@@             Coverage Diff              @@
##                2.x    #4003      +/-   ##
============================================
- Coverage     70.63%   70.53%   -0.10%     
- Complexity    56487    56514      +27     
============================================
  Files          4527     4532       +5     
  Lines        271798   272037     +239     
  Branches      39975    39987      +12     
============================================
- Hits         191985   191887      -98     
- Misses        63718    64036     +318     
- Partials      16095    16114      +19     
Impacted Files Coverage Δ
...g/opensearch/index/engine/EngineConfigFactory.java 88.63% <ø> (ø)
...va/org/opensearch/index/engine/ReadOnlyEngine.java 71.62% <0.00%> (+0.87%) ⬆️
...in/java/org/opensearch/indices/IndicesService.java 69.90% <0.00%> (+2.41%) ⬆️
.../main/java/org/opensearch/index/engine/Engine.java 74.05% <50.00%> (+<0.01%) ⬆️
...va/org/opensearch/index/engine/InternalEngine.java 74.98% <55.55%> (-0.28%) ⬇️
...java/org/opensearch/index/engine/EngineConfig.java 93.33% <57.14%> (-3.77%) ⬇️
...in/java/org/opensearch/index/shard/IndexShard.java 70.32% <60.00%> (+1.16%) ⬆️
.../opensearch/index/engine/NRTReplicationEngine.java 62.57% <62.57%> (ø)
.../indices/replication/common/ReplicationTarget.java 63.41% <63.41%> (ø)
...ch/indices/recovery/PeerRecoveryTargetService.java 52.18% <68.00%> (-0.18%) ⬇️
... and 517 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

…ava and adressing comments from previous PR

Signed-off-by: Rishikesh1159 <[email protected]>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@Rishikesh1159 Rishikesh1159 merged commit 053cf55 into opensearch-project:2.x Jul 28, 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.

5 participants