Toggle replication strategy based on segrep index setting #2318

Poojita-Raj · 2022-03-03T00:14:31Z

Description

Docrep and segrep should be able to work in parallel with this change. Previously, commented code was breaking docrep

test docrep by running IndexRecoveryIT

Resolves #2197

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.

opensearch-ci-bot · 2022-03-03T00:16:03Z

Can one of the admins verify this patch?

mch2

Looks like a great start!

We will also need to conditionally wire up the CheckpointRefreshListener here so that checkpoints are not published on primary refresh. We should probably also add a condition in IndexShard's onNewCheckpoint method to be safe.

mch2 · 2022-03-03T00:20:24Z

server/src/main/java/org/opensearch/index/engine/InternalEngine.java

@@ -277,17 +277,25 @@ public InternalEngine(EngineConfig engineConfig) {
                );
                this.localCheckpointTracker = createLocalCheckpointTracker(localCheckpointTrackerSupplier);
                // TODO: Segrep - should have a separate read only engine rather than all this conditional logic.
-                if (engineConfig.isPrimary()) {
+                if (engineConfig.getIndexSettings().isSegrepEnabled()) {


Wiring up primary shards is the same in both cases. We can remove some duplication here with:

if (engineConfig.getIndexSettings().isSegrepEnabled && engineConfig.isPrimary() == false) { ... setup replica } else { ..setup primary }

mch2 · 2022-03-03T00:23:30Z

server/src/main/java/org/opensearch/index/engine/OpenSearchReaderManager.java

-import org.apache.lucene.index.LeafReaderContext;
-import org.apache.lucene.index.SegmentInfos;
-import org.apache.lucene.index.StandardDirectoryReader;
+import org.apache.lucene.index.*;


we should leave the specific imports as is here. My Intellij sometimes does this by default and had to change in Preferences > code style > java and mark use single class import and change `Class count to use import with '*' to a high number.

mch2 · 2022-03-03T00:25:13Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

@@ -3910,8 +3912,10 @@ ReplicationTracker getReplicationTracker() {
    public boolean scheduledRefresh() {
        // skip if not primary shard.
        // TODO: Segrep - should split into primary/replica classes.
-        if (shardRouting.primary() == false) {
-            return false;
+        if (indexSettings.isSegrepEnabled()) {


nit - can be on one line.

opensearch-ci-bot · 2022-03-03T01:02:58Z

❌ Gradle Check failure 6b3809d751aa6ddb8fce85fb52259e7c10652152
Log 2949

Reports 2949

opensearch-ci-bot · 2022-03-04T17:20:43Z

❌ Gradle Check failure 7f8e55dd04fd56375a7f99562e6f5bd44644d81f
Log 3035

Reports 3035

opensearch-ci-bot · 2022-03-05T01:53:05Z

❌ Gradle Check failure 5950974c5a2d3f93290f82052469054a4331db8c
Log 3069

Reports 3069

opensearch-ci-bot · 2022-03-07T18:23:10Z

❌ Gradle Check failure 7ff8fa8f5aed46bc988e83d67e44684cef749d3e
Log 3119

Reports 3119

mch2 · 2022-03-07T18:19:49Z

server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java

@@ -259,6 +259,17 @@ public void validate(final Integer numRoutingShards, final Map<Setting<?>, Objec
        Property.IndexScope
    );

+    public static final String SETTING_SEGMENT_REPLICATION = "index.replication.segment_replication";


Needs a rebase here, the setting changes were merged.

mch2 · 2022-03-07T18:23:52Z

server/src/main/java/org/opensearch/index/engine/InternalEngine.java

@@ -277,17 +277,17 @@ public InternalEngine(EngineConfig engineConfig) {
                );
                this.localCheckpointTracker = createLocalCheckpointTracker(localCheckpointTrackerSupplier);
                // TODO: Segrep - should have a separate read only engine rather than all this conditional logic.
-                if (engineConfig.isPrimary()) {
+                if ((engineConfig.getIndexSettings().isSegrepEnabled()) && engineConfig.isPrimary() == false) {


I think we should take this opportunity to rename isPrimary() to isReadOnly(), where replicas are readOnly. This is a stepping stone to further refactoring into a read only engine implementation.

mch2 · 2022-03-07T18:26:03Z

server/src/main/java/org/opensearch/index/engine/InternalEngine.java

@@ -582,7 +582,8 @@ private void recoverFromTranslogInternal(TranslogRecoveryRunner translogRecovery
                translog.currentFileGeneration()
            )
        );
-        if (engineConfig.isPrimary()) {
+        boolean segrep = engineConfig.getIndexSettings().isSegrepEnabled();
+        if ((segrep == false) || (segrep && engineConfig.isPrimary())) {


this condition shows up ~5 times, lets pull out a small private method, something like isReadOnlyReplica() == false.

mch2 · 2022-03-07T18:28:23Z

server/src/main/java/org/opensearch/index/engine/OpenSearchReaderManager.java

@@ -32,10 +32,6 @@

 package org.opensearch.index.engine;

-import java.io.IOException;


Was this reorder intentional? Think this may be IDE.

I left it in since this seems to be the standard order followed by all other files

mch2 · 2022-03-07T18:37:27Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

@@ -2067,7 +2068,9 @@ private void innerOpenEngineAndTranslog(LongSupplier globalCheckpointSupplier) t
        // which settings changes could possibly have happened, so here we forcefully push any config changes to the new engine.
        onSettingsChanged();
        // TODO: Segrep - Fix
-        // assert assertSequenceNumbersInCommit();


I think this method call may be ok to uncomment now with segrep enabled. We can take that as a separate change.

mch2 · 2022-03-07T18:39:38Z

server/src/main/java/org/opensearch/index/shard/IndexShard.java

@@ -2715,7 +2718,9 @@ public void syncRetentionLeases() {
        assert assertPrimaryMode();
        verifyNotClosed();
        // TODO: Segrep - Fix retention leases
-        // replicationTracker.renewPeerRecoveryRetentionLeases();


Leaving a note here similar to the above - I think this may be ok to uncomment now with segrep enabled. This was commented before we were correctly wiring up retention leases for replicas.

Signed-off-by: Poojita Raj <[email protected]> index writer null exception Signed-off-by: Poojita Raj <[email protected]> included conditional logic Signed-off-by: Poojita Raj <[email protected]> internal listener fix Signed-off-by: Poojita Raj <[email protected]>

Signed-off-by: Poojita Raj <[email protected]>

opensearch-ci-bot · 2022-03-07T23:48:11Z

❌ Gradle Check failure e8437bd
Log 3147

Reports 3147

mch2 · 2022-03-08T01:18:55Z

server/src/main/java/org/opensearch/index/IndexSettings.java

@@ -883,7 +883,7 @@ public int getNumberOfReplicas() {
    }

    public boolean isSegrepEnabled() {
-        return isSegrepEnabled;
+        return Boolean.TRUE.equals(isSegrepEnabled);


nit - this isn't required bc we always initialize this variable in the constructor.
isSegrepEnabled = settings.getAsBoolean(IndexMetadata.SETTING_SEGMENT_REPLICATION, false);

True but leaving it in to convert to boolean which is faster since we don't need a thread safe implementation.

mch2 · 2022-03-08T01:20:03Z

server/src/main/java/org/opensearch/index/engine/InternalEngine.java

@@ -346,7 +350,7 @@ public InternalEngine(EngineConfig engineConfig) {

    @Override
    public void updateCurrentInfos(byte[] infosBytes, long gen, long seqNo) throws IOException {
-        assert engineConfig.isPrimary() == false : "Only replicas should update Infos";
+        assert engineConfig.isReadOnly() == true : "Only replicas should update Infos";


nit - this failure message is a bit confusing with the rename. Perhaps 'Only read only replicas should update Infos'

opensearch-ci-bot · 2022-03-08T21:06:08Z

❌ Gradle Check failure 3d0124e
Log 3186

Reports 3186

mch2 reviewed Mar 3, 2022

View reviewed changes

Poojita-Raj force-pushed the feature/segment-replication branch from 7f8e55d to 5950974 Compare March 5, 2022 01:44

Poojita-Raj marked this pull request as ready for review March 7, 2022 17:48

Poojita-Raj requested a review from a team as a code owner March 7, 2022 17:48

Poojita-Raj changed the title ~~Feature/segment replication~~ Toggle replication strategy based on segrep index setting Mar 7, 2022

mch2 reviewed Mar 7, 2022

View reviewed changes

Poojita-Raj added 3 commits March 7, 2022 11:41

toggle

cd03754

Signed-off-by: Poojita Raj <[email protected]> index writer null exception Signed-off-by: Poojita Raj <[email protected]> included conditional logic Signed-off-by: Poojita Raj <[email protected]> internal listener fix Signed-off-by: Poojita Raj <[email protected]>

docrep working

56ef936

Signed-off-by: Poojita Raj <[email protected]>

refactor isPrimary logic to isReadOnly

e8437bd

Signed-off-by: Poojita Raj <[email protected]>

Poojita-Raj force-pushed the feature/segment-replication branch from 7ff8fa8 to e8437bd Compare March 7, 2022 23:20

mch2 approved these changes Mar 8, 2022

View reviewed changes

Update assert exception message

3d0124e

Poojita-Raj merged commit 9341099 into opensearch-project:feature/segment-replication Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggle replication strategy based on segrep index setting #2318

Toggle replication strategy based on segrep index setting #2318

Poojita-Raj commented Mar 3, 2022 •

edited

Loading

opensearch-ci-bot commented Mar 3, 2022

mch2 left a comment

mch2 Mar 3, 2022

mch2 Mar 3, 2022

mch2 Mar 3, 2022

opensearch-ci-bot commented Mar 3, 2022

opensearch-ci-bot commented Mar 4, 2022

opensearch-ci-bot commented Mar 5, 2022

opensearch-ci-bot commented Mar 7, 2022

mch2 Mar 7, 2022

mch2 Mar 7, 2022

mch2 Mar 7, 2022

Poojita-Raj Mar 7, 2022

mch2 Mar 7, 2022

Poojita-Raj Mar 7, 2022

mch2 Mar 7, 2022

mch2 Mar 7, 2022

opensearch-ci-bot commented Mar 7, 2022

mch2 Mar 8, 2022

Poojita-Raj Mar 8, 2022

mch2 Mar 8, 2022

opensearch-ci-bot commented Mar 8, 2022

		@@ -32,10 +32,6 @@

		package org.opensearch.index.engine;

		import java.io.IOException;

Toggle replication strategy based on segrep index setting #2318

Toggle replication strategy based on segrep index setting #2318

Conversation

Poojita-Raj commented Mar 3, 2022 • edited Loading

Description

Check List

opensearch-ci-bot commented Mar 3, 2022

mch2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 3, 2022

opensearch-ci-bot commented Mar 4, 2022

opensearch-ci-bot commented Mar 5, 2022

opensearch-ci-bot commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-ci-bot commented Mar 8, 2022

Poojita-Raj commented Mar 3, 2022 •

edited

Loading