address review comments

Signed-off-by: Poojita Raj <[email protected]>
opensearch-project · Jul 17, 2023 · 9666949 · 9666949
1 parent a40c6a5
commit 9666949
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 12 deletions.
diff --git a/server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java b/server/src/main/java/org/opensearch/index/engine/NRTReplicationEngine.java
@@ -114,7 +114,7 @@ public void onAfterTranslogSync() {
                 engineConfig.getPrimaryModeSupplier()
             );
             this.translogManager = translogManagerRef;
-            this.shouldCommit = !engineConfig.getIndexSettings().isRemoteStoreEnabled();
+            this.shouldCommit = engineConfig.getIndexSettings().isRemoteStoreEnabled() == false;
         } catch (IOException e) {
             IOUtils.closeWhileHandlingException(store::decRef, readerManager, translogManagerRef);
             throw new EngineCreationFailureException(shardId, "failed to create engine", e);
@@ -145,12 +145,7 @@ public synchronized void updateSegments(final SegmentInfos infos) throws IOExcep
             // Commit and roll the translog when we receive a different generation than what was last received.
             // lower/higher gens are possible from a new primary that was just elected.
             if (incomingGeneration != lastReceivedGen) {
-                if (shouldCommit) {
-                    commitSegmentInfos();
-                } else {
-                    this.lastCommittedSegmentInfos = store.readLastCommittedSegmentsInfo();
-                    translogManager.syncTranslog();
-                }
+                commitSegmentInfos();
                 translogManager.getDeletionPolicy().setLocalCheckpointOfSafeCommit(maxSeqNo);
                 translogManager.rollTranslogGeneration();
             }
@@ -170,7 +165,9 @@ public synchronized void updateSegments(final SegmentInfos infos) throws IOExcep
      * @throws IOException - When there is an IO error committing the SegmentInfos.
      */
     private void commitSegmentInfos(SegmentInfos infos) throws IOException {
-        store.commitSegmentInfos(infos, localCheckpointTracker.getMaxSeqNo(), localCheckpointTracker.getProcessedCheckpoint());
+        if (shouldCommit) {
+            store.commitSegmentInfos(infos, localCheckpointTracker.getMaxSeqNo(), localCheckpointTracker.getProcessedCheckpoint());
+        }
         this.lastCommittedSegmentInfos = store.readLastCommittedSegmentsInfo();
         translogManager.syncTranslog();
     }

diff --git a/server/src/main/java/org/opensearch/index/shard/IndexShard.java b/server/src/main/java/org/opensearch/index/shard/IndexShard.java
@@ -4643,16 +4643,15 @@ public void syncSegmentsFromRemoteSegmentStore(boolean overrideLocal, boolean re
                         indexInput,
                         remoteSegmentMetadata.getGeneration()
                     );
+                    // Replicas never need a local commit
                     if (shouldCommit) {
-                        // Replicas never need a local commit
                         if (this.shardRouting.primary()) {
                             long processedLocalCheckpoint = Long.parseLong(infosSnapshot.getUserData().get(LOCAL_CHECKPOINT_KEY));
                             // Following code block makes sure to use SegmentInfosSnapshot in the remote store if generation differs
                             // with local filesystem. If local filesystem already has segments_N+2 and infosSnapshot has generation N,
                             // after commit, there would be 2 files that would be created segments_N+1 and segments_N+2. With the
                             // policy of preserving only the latest commit, we will delete segments_N+1 which in fact is the part of the
-                            // latest
-                            // commit.
+                            // latest commit.
                             Optional<String> localMaxSegmentInfos = localSegmentFiles.stream()
                                 .filter(file -> file.startsWith(IndexFileNames.SEGMENTS))
                                 .max(Comparator.comparingLong(SegmentInfos::generationFromSegmentsFileName));

diff --git a/.../test/java/org/opensearch/indices/recovery/RemoteStorePeerRecoverySourceHandlerTests.java b/.../test/java/org/opensearch/indices/recovery/RemoteStorePeerRecoverySourceHandlerTests.java
@@ -63,7 +63,6 @@ public void testReplicaShardRecoveryUptoLastFlushedCommit() throws Exception {
             // Step 7 - Check retention lease does not exist for the replica shard
             assertEquals(1, primary.getRetentionLeases().leases().size());
             assertFalse(primary.getRetentionLeases().contains(ReplicationTracker.getPeerRecoveryRetentionLeaseId(replica2.routingEntry())));
-
         }
     }
 }