From 96669495d651af9c0d4ebb52071841523d5b1054 Mon Sep 17 00:00:00 2001 From: Poojita Raj Date: Sun, 16 Jul 2023 17:40:13 -0700 Subject: [PATCH] address review comments Signed-off-by: Poojita Raj --- .../index/engine/NRTReplicationEngine.java | 13 +++++-------- .../java/org/opensearch/index/shard/IndexShard.java | 5 ++--- .../RemoteStorePeerRecoverySourceHandlerTests.java | 1 - 3 files changed, 7 insertions(+), 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 index 93835900756cf..f2678219f34b2 100644 --- 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 index 4f61e987a3dd2..3dd8a673bf779 100644 --- 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 localMaxSegmentInfos = localSegmentFiles.stream() .filter(file -> file.startsWith(IndexFileNames.SEGMENTS)) .max(Comparator.comparingLong(SegmentInfos::generationFromSegmentsFileName)); diff --git a/server/src/test/java/org/opensearch/indices/recovery/RemoteStorePeerRecoverySourceHandlerTests.java b/server/src/test/java/org/opensearch/indices/recovery/RemoteStorePeerRecoverySourceHandlerTests.java index 51b1a41b55b24..96022315743c2 100644 --- a/server/src/test/java/org/opensearch/indices/recovery/RemoteStorePeerRecoverySourceHandlerTests.java +++ b/server/src/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()))); - } } }