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

For segrep enabled indices, use NRTReplicationEngine for replica shards #486

Merged
merged 3 commits into from
Aug 29, 2022

Conversation

dreamer-89
Copy link
Member

@dreamer-89 dreamer-89 commented Aug 19, 2022

Signed-off-by: Suraj Singh surajrider@gmail.com

Description

Update EngineFactory to return NRTReplicationEngine for SegRep enabled replica shards. Today both primary & replica uses ReplicationEngine (extends InternalEngine from OpenSearch) though NRTReplicationEngine is needed on replica shards for SegmentReplication to work.

Resolves

opensearch-project/OpenSearch#3823

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: Suraj Singh <surajrider@gmail.com>
@dreamer-89 dreamer-89 changed the title Update engine factory to return NRTReplicationEngine for replica shards For segrep enabled indices, use NRTReplicationEngine for replica shards Aug 19, 2022
Signed-off-by: Suraj Singh <surajrider@gmail.com>
ankitkala
ankitkala previously approved these changes Aug 23, 2022
@ankitkala ankitkala enabled auto-merge (squash) August 23, 2022 01:50
Optional.of(EngineFactory { config -> ReplicationEngine(config) })
Optional.of(EngineFactory { config ->
// Use NRTSegmentReplicationEngine for SEGMENT replication type indices replica shards
if (config.isReadOnlyReplica && indexSettings.settings.get(INDEX_REPLICATION_TYPE_SETTING.key) != null && indexSettings.settings.get(INDEX_REPLICATION_TYPE_SETTING.key).equals("SEGMENT")) {
Copy link
Member

Choose a reason for hiding this comment

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

  • we can drop explicit null check and use "?" instead for obtaining replication type setting.
  • Can we use constant for "SEGMENT" (from the allowed values)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like config.isReadOnlyReplica already have segrep settings check inside EngineConfig. Removing segrep setting explicit check.

    public boolean isReadOnlyReplica() {
        return indexSettings.isSegRepEnabled() && isReadOnlyReplica;
    }

Signed-off-by: Suraj Singh <surajrider@gmail.com>
@codecov-commenter
Copy link

Codecov Report

Merging #486 (555e412) into main (a859596) will decrease coverage by 2.11%.
The diff coverage is 71.11%.

❗ Current head 555e412 differs from pull request most recent head 4ba2d80. Consider uploading reports for the commit 4ba2d80 to get more accurate results

@@             Coverage Diff              @@
##               main     #486      +/-   ##
============================================
- Coverage     75.16%   73.05%   -2.12%     
+ Complexity     1007      983      -24     
============================================
  Files           141      141              
  Lines          4579     4595      +16     
  Branches        506      506              
============================================
- Hits           3442     3357      -85     
- Misses          823      925     +102     
+ Partials        314      313       -1     
Impacted Files Coverage Δ
...in/org/opensearch/replication/ReplicationPlugin.kt 90.41% <50.00%> (-1.20%) ⬇️
...action/stop/TransportStopIndexReplicationAction.kt 60.75% <50.00%> (-13.91%) ⬇️
...rch/replication/task/index/IndexReplicationTask.kt 67.66% <75.00%> (-3.41%) ⬇️
...arch/replication/task/autofollow/AutoFollowTask.kt 71.89% <100.00%> (-1.08%) ⬇️
...arch/replication/metadata/UpdateMetadataRequest.kt 77.27% <0.00%> (-22.73%) ⬇️
...ch/replication/task/index/IndexReplicationState.kt 51.66% <0.00%> (-15.00%) ⬇️
...ication/action/setup/TransportSetupChecksAction.kt 59.52% <0.00%> (-14.29%) ⬇️
...ication/seqno/RemoteClusterRetentionLeaseHelper.kt 80.00% <0.00%> (-11.67%) ⬇️
...tion/repository/RemoteClusterMultiChunkTransfer.kt 90.24% <0.00%> (-9.76%) ⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ankitkala ankitkala merged commit 81c2002 into opensearch-project:main Aug 29, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 29, 2022
…ds (#486)

* Update engine factory to return NRTReplicationEngine for replica shards

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Address review comments

Signed-off-by: Suraj Singh <surajrider@gmail.com>

Signed-off-by: Suraj Singh <surajrider@gmail.com>
(cherry picked from commit 81c2002)
ankitkala pushed a commit that referenced this pull request Aug 30, 2022
…ds (#486) (#533)

* Update engine factory to return NRTReplicationEngine for replica shards

Signed-off-by: Suraj Singh <surajrider@gmail.com>

* Address review comments

Signed-off-by: Suraj Singh <surajrider@gmail.com>

Signed-off-by: Suraj Singh <surajrider@gmail.com>
(cherry picked from commit 81c2002)

Co-authored-by: Suraj Singh <surajrider@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants