Refactor translog download flow and Add support to run SegRep integ tests using remote store settings #6405

sachinpkale · 2023-02-21T10:40:00Z

Description

We want to test various scenarios involving replica promotion, peer recovery, index close and re-open with remote store.
These scenarios are already being tested in various integ tests of Segment Replication.
In this change, we re-use the same tests by enabling remote store settings.

Issues Resolved

[BUG] Assertion failure in IndexShard.updateGlobalCheckpointOnReplica() when remote translog is enabled #6193
[BUG] Assertion failure in replica promotion using remote translog #6195

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
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

github-actions · 2023-02-21T11:00:21Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11487/
CommitID: c74df42
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-02-21T11:20:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11488/
CommitID: 3f3bfc6
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-02-21T12:50:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11494/
CommitID: 9b3a4f0
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-01T08:06:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11854/
CommitID: ed57c70
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-01T11:18:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/11858/
CommitID: d934bdc
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

sachinpkale · 2023-03-01T12:30:56Z

Following test is failing, looking into it.

[org.opensearch.index.translog.RemoteFSTranslogTests.testMetadataFileDeletion]

github-actions · 2023-03-10T07:43:33Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12226/
CommitID: 69172da
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-03-20T08:02:45Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      2 org.opensearch.cluster.service.MasterServiceTests.classMethod
      1 org.opensearch.indices.replication.SegmentReplicationRelocationIT.testPrimaryRelocation
      1 org.opensearch.cluster.service.MasterServiceTests.testThrottlingForMultipleTaskTypes
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

URL: https://build.ci.opensearch.org/job/gradle-check/12655/
CommitID: 897d263
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov-commenter · 2023-03-20T08:03:56Z

Codecov Report

Merging #6405 (0fa7a9c) into main (8c9cfdc) will increase coverage by 0.11%.
The diff coverage is 76.47%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #6405      +/-   ##
============================================
+ Coverage     70.73%   70.85%   +0.11%     
- Complexity    59281    59313      +32     
============================================
  Files          4812     4812              
  Lines        283614   283641      +27     
  Branches      40896    40900       +4     
============================================
+ Hits         200606   200962     +356     
+ Misses        66552    66201     -351     
- Partials      16456    16478      +22

Impacted Files	Coverage Δ
...java/org/opensearch/index/shard/StoreRecovery.java	`67.39% <33.33%> (+1.43%)`	⬆️
...in/java/org/opensearch/index/shard/IndexShard.java	`69.83% <53.33%> (-0.10%)`	⬇️
...va/org/opensearch/index/engine/ReadOnlyEngine.java	`72.34% <66.66%> (+0.19%)`	⬆️
...dex/translog/transfer/TranslogTransferManager.java	`81.43% <87.50%> (-1.18%)`	⬇️
...rg/opensearch/index/translog/RemoteFsTranslog.java	`76.40% <95.23%> (+0.77%)`	⬆️
...anslog/RemoteBlobStoreInternalTranslogFactory.java	`76.92% <100.00%> (+1.92%)`	⬆️

... and 459 files with indirect coverage changes

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

Bukhtawar · 2023-03-24T17:32:08Z

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

+    public void syncTranslogFilesFromRemoteTranslog() throws IOException {
+        TranslogFactory translogFactory = translogFactorySupplier.apply(indexSettings, shardRouting);
+        assert translogFactory instanceof RemoteBlobStoreInternalTranslogFactory;
+        Repository repository = ((RemoteBlobStoreInternalTranslogFactory) translogFactory).getRepository();
+        assert repository instanceof BlobStoreRepository : "repository should be instance of BlobStoreRepository";
+        BlobStoreRepository blobStoreRepository = (BlobStoreRepository) repository;
+        FileTransferTracker fileTransferTracker = new FileTransferTracker(shardId);
+        TranslogTransferManager translogTransferManager = RemoteFsTranslog.buildTranslogTransferManager(
+            blobStoreRepository,
+            getThreadPool(),
+            shardId,
+            fileTransferTracker
+        );
+        RemoteFsTranslog.download(translogTransferManager, shardPath().resolveTranslog());
+    }


This entire logic should sit in RemoteFsTranslog and IndexShard should ever interact with a common Translog interface. Otherwise every consumer will need to be aware of the underlying translog which will make the interfaces too tightly coupled.

Makes sense, will make the changes accordingly.

Tracking it here: #5679

Signed-off-by: Sachin Kale <kalsac@amazon.com>

github-actions · 2023-03-27T08:32:28Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/12942/
CommitID: af69749
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

Signed-off-by: Sachin Kale <kalsac@amazon.com>

github-actions · 2023-03-27T11:48:35Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.smoketest.SmokeTestMultiNodeClientYamlTestSuiteIT.test {yaml=pit/10_basic/Delete all}

URL: https://build.ci.opensearch.org/job/gradle-check/12945/
CommitID: 0fa7a9c
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

ashking94 · 2023-03-28T09:00:03Z

...src/internalClusterTest/java/org/opensearch/remotestore/SegmentReplicationRemoteStoreIT.java

+
+import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
+
+@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0)


Just a question - what numDataNodes = 0 means here? I checked the code base and saw multiple such occurrences.

We want to control number of nodes in the test cluster. That is why we provide 0 and start nodes as per the test requirement.

More on numDataNodes: https://github.com/opensearch-project/OpenSearch/blob/main/test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java#L1722

ashking94 · 2023-03-28T09:06:00Z

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

@@ -137,7 +137,9 @@ public ReadOnlyEngine(
                }
                if (seqNoStats == null) {
                    seqNoStats = buildSeqNoStats(config, lastCommittedSegmentInfos);
-                    ensureMaxSeqNoEqualsToGlobalCheckpoint(seqNoStats);


Nit - would it make this code more readable, if we offload this check to within ensureMaxSeqNoEqualsToGlobalCheckpoint method -

if (requireCompleteHistory == false || engineConfig.getIndexSettings().isRemoteTranslogStoreEnabled()) { return; }

OpenSearch/server/src/main/java/org/opensearch/index/engine/ReadOnlyEngine.java

Lines 179 to 181 in 5f89081

protected void ensureMaxSeqNoEqualsToGlobalCheckpoint(final SeqNoStats seqNoStats) {

if (requireCompleteHistory == false) {

return;

ashking94 · 2023-03-28T09:08:20Z

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

@@ -186,7 +188,7 @@ protected void ensureMaxSeqNoEqualsToGlobalCheckpoint(final SeqNoStats seqNoStat
        // In addition to that we only execute the check if the index the engine belongs to has been
        // created after the refactoring of the Close Index API and its TransportVerifyShardBeforeCloseAction
        // that guarantee that all operations have been flushed to Lucene.
-        assert assertMaxSeqNoEqualsToGlobalCheckpoint(seqNoStats.getMaxSeqNo(), seqNoStats.getGlobalCheckpoint());
+        assertMaxSeqNoEqualsToGlobalCheckpoint(seqNoStats.getMaxSeqNo(), seqNoStats.getGlobalCheckpoint());


can we also change the contract of the method to return void since the return value is not being used any more?

ashking94 · 2023-03-28T09:13:00Z

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

@@ -3079,7 +3085,8 @@ public void updateGlobalCheckpointOnReplica(final long globalCheckpoint, final S
             * while the global checkpoint update may have emanated from the primary when we were in that state, we could subsequently move
             * to recovery finalization, or even finished recovery before the update arrives here.
             */
-            assert state() != IndexShardState.POST_RECOVERY && state() != IndexShardState.STARTED
+            assert (state() != IndexShardState.POST_RECOVERY && state() != IndexShardState.STARTED)
+                || (indexSettings.isRemoteTranslogStoreEnabled() == true && state() != IndexShardState.RECOVERING)


Should we add a short comment or add to method's java doc on why this condition has been added?

ashking94 · 2023-03-28T09:22:10Z

server/src/main/java/org/opensearch/index/translog/RemoteFsTranslog.java

@@ -141,6 +154,25 @@ public static void download(TranslogTransferManager translogTransferManager, Pat
        }
    }

+    private static void deleteTranslogFilesNotUploaded(Path location, long uploadedGeneration) throws IOException {


If I recall the earlier check on deleting files before downloading translog files was added as otherwise the download used to fail, right? Now we are deleting files that are greater than the max generation referenced by the remote translog metadata file. While this is really a good optimisation, I think we should check the local file's checksum and the expected checksum. If there is a story on the checksum part already, then do let me know. If not, let's follow it up in next PR.

On high level, comparing checksum makes sense. This is what we do for segments and can be added here as well.
The only reason I have not added this check was due to the invariant that we have: only one node will be uploading a translog file at any given time. So, if the translog file name matches, checksum must be same. Let me create a tracking issue to discuss this in more detail.

Created tracking issue: #6896

ashking94

Reviewed on high level. Do take a look at the comments and address.

Signed-off-by: Sachin Kale <kalsac@amazon.com>

github-actions · 2023-03-30T14:17:47Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.split/30_copy_settings/Copy settings during split index}

URL: https://build.ci.opensearch.org/job/gradle-check/13102/
CommitID: 66fbbcc
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

sachinpkale · 2023-04-04T13:06:33Z

This PR is trying to solve multiple things. Breaking it down into multiple PRs

sachinpkale added the skip-changelog label Feb 21, 2023

sachinpkale mentioned this pull request Feb 21, 2023

[Remote Store] Add support to run SegRep integ tests using remote store settings #6192

Closed

6 tasks

sachinpkale force-pushed the refactor-translog-download branch from 9b3a4f0 to ed57c70 Compare March 1, 2023 07:45

sachinpkale force-pushed the refactor-translog-download branch from d934bdc to 409c78e Compare March 6, 2023 09:32

sachinpkale marked this pull request as ready for review March 6, 2023 09:35

sachinpkale requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja and dreamer-89 as code owners March 6, 2023 09:35

sachinpkale force-pushed the refactor-translog-download branch from 69172da to 897d263 Compare March 20, 2023 07:33

sachinpkale requested review from andrross and removed request for adnapibar March 20, 2023 07:44

Bukhtawar reviewed Mar 24, 2023

View reviewed changes

Sachin Kale added 7 commits March 27, 2023 11:52

Add support to run SegRep integ tests using remote store settings

61ef980

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Apply Spotless fixes

7c2cfc5

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Remove redundant code

39b6573

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Fix spotless

ed97a69

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Fix test

035db36

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Address PR comments

079137f

Signed-off-by: Sachin Kale <kalsac@amazon.com>

Incorporate PR comments

af69749

Signed-off-by: Sachin Kale <kalsac@amazon.com>

sachinpkale force-pushed the refactor-translog-download branch from 897d263 to af69749 Compare March 27, 2023 08:07

Fix spotless error

0fa7a9c

Signed-off-by: Sachin Kale <kalsac@amazon.com>

ashking94 reviewed Mar 28, 2023

View reviewed changes

ashking94 requested changes Mar 28, 2023

View reviewed changes

Address PR comments

66fbbcc

Signed-off-by: Sachin Kale <kalsac@amazon.com>

sachinpkale mentioned this pull request Mar 30, 2023

[Remote Translog] Consider checksum along with filename while identifying a translog/ckp file #6896

Open

sachinpkale marked this pull request as draft April 4, 2023 13:06

sachinpkale closed this May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor translog download flow and Add support to run SegRep integ tests using remote store settings #6405

Refactor translog download flow and Add support to run SegRep integ tests using remote store settings #6405

sachinpkale commented Feb 21, 2023 •

edited

Loading

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Feb 21, 2023

github-actions bot commented Mar 1, 2023

github-actions bot commented Mar 1, 2023

sachinpkale commented Mar 1, 2023

github-actions bot commented Mar 10, 2023

github-actions bot commented Mar 20, 2023

codecov-commenter commented Mar 20, 2023 •

edited

Loading

Bukhtawar Mar 24, 2023

sachinpkale Mar 27, 2023

sachinpkale Mar 30, 2023

github-actions bot commented Mar 27, 2023

github-actions bot commented Mar 27, 2023

ashking94 Mar 28, 2023

sachinpkale Mar 30, 2023

ashking94 Mar 28, 2023 •

edited

Loading

ashking94 Mar 28, 2023

ashking94 Mar 28, 2023

ashking94 Mar 28, 2023

sachinpkale Mar 30, 2023

sachinpkale Mar 30, 2023 •

edited

Loading

ashking94 left a comment

github-actions bot commented Mar 30, 2023

sachinpkale commented Apr 4, 2023


		import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;

		@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0)

	protected void ensureMaxSeqNoEqualsToGlobalCheckpoint(final SeqNoStats seqNoStats) {
	if (requireCompleteHistory == false) {
	return;

Refactor translog download flow and Add support to run SegRep integ tests using remote store settings #6405

Refactor translog download flow and Add support to run SegRep integ tests using remote store settings #6405

Conversation

sachinpkale commented Feb 21, 2023 • edited Loading

Description

Issues Resolved

Check List

github-actions bot commented Feb 21, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 21, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Feb 21, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 1, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 1, 2023

Gradle Check (Jenkins) Run Completed with:

sachinpkale commented Mar 1, 2023

github-actions bot commented Mar 10, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 20, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Mar 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Mar 27, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashking94 Mar 28, 2023 • edited Loading

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

sachinpkale Mar 30, 2023 • edited Loading

Choose a reason for hiding this comment

ashking94 left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 30, 2023

Gradle Check (Jenkins) Run Completed with:

sachinpkale commented Apr 4, 2023

sachinpkale commented Feb 21, 2023 •

edited

Loading

codecov-commenter commented Mar 20, 2023 •

edited

Loading

ashking94 Mar 28, 2023 •

edited

Loading

sachinpkale Mar 30, 2023 •

edited

Loading