Address PR Comments

Signed-off-by: Harish Bhakuni <hbhakuni@amazon.com>

Author: Harish Bhakuni

server/src/main/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshot.java

@@ -17,7 +17,6 @@
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Objects;
 
 /**
  * Remote Store based Shard snapshot metadata
@@ -40,7 +39,7 @@ public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment {
     private final String indexUUID;
     private final List<String> fileNames;
 
-    private static final String DEFAULT_VERSION = "1";
+    static final String DEFAULT_VERSION = "1";
     static final String NAME = "name";
     static final String VERSION = "version";
     static final String INDEX_VERSION = "index_version";
@@ -116,22 +115,69 @@ public RemoteStoreShardShallowCopySnapshot(
         String repositoryBasePath,
         List<String> fileNames
     ) {
-        this.snapshot = Objects.requireNonNull(snapshot, "Invalid/Missing Snapshot Name");
-        assert indexVersion > 0 : "Invalid Index Version";
-        assert primaryTerm > 0 : "Invalid Primary Term";
-        assert commitGeneration > 0 : "Invalid Commit Generation";
+        this.version = DEFAULT_VERSION;
+        verifyParameters(
+            version,
+            snapshot,
+            indexVersion,
+            primaryTerm,
+            commitGeneration,
+            indexUUID,
+            remoteStoreRepository,
+            repositoryBasePath
+        );
+        this.snapshot = snapshot;
         this.indexVersion = indexVersion;
         this.primaryTerm = primaryTerm;
         this.commitGeneration = commitGeneration;
         this.startTime = startTime;
         this.time = time;
         this.totalFileCount = totalFileCount;
         this.totalSize = totalSize;
-        this.indexUUID = Objects.requireNonNull(indexUUID, "Invalid/Missing Index UUID");
-        this.remoteStoreRepository = Objects.requireNonNull(remoteStoreRepository, "Invalid/Missing Remote Store Repository");
-        this.repositoryBasePath = Objects.requireNonNull(repositoryBasePath, "Invalid/Missing Repository Base Path");
+        this.indexUUID = indexUUID;
+        this.remoteStoreRepository = remoteStoreRepository;
+        this.repositoryBasePath = repositoryBasePath;
+        this.fileNames = fileNames;
+    }
+
+    private RemoteStoreShardShallowCopySnapshot(
+        String version,
+        String snapshot,
+        long indexVersion,
+        long primaryTerm,
+        long commitGeneration,
+        long startTime,
+        long time,
+        int totalFileCount,
+        long totalSize,
+        String indexUUID,
+        String remoteStoreRepository,
+        String repositoryBasePath,
+        List<String> fileNames
+    ) {
+        verifyParameters(
+            version,
+            snapshot,
+            indexVersion,
+            primaryTerm,
+            commitGeneration,
+            indexUUID,
+            remoteStoreRepository,
+            repositoryBasePath
+        );
+        this.version = version;
+        this.snapshot = snapshot;
+        this.indexVersion = indexVersion;
+        this.primaryTerm = primaryTerm;
+        this.commitGeneration = commitGeneration;
+        this.startTime = startTime;
+        this.time = time;
+        this.totalFileCount = totalFileCount;
+        this.totalSize = totalSize;
+        this.indexUUID = indexUUID;
+        this.remoteStoreRepository = remoteStoreRepository;
+        this.repositoryBasePath = repositoryBasePath;
         this.fileNames = fileNames;
-        this.version = DEFAULT_VERSION;
     }
 
     /**
@@ -142,7 +188,7 @@ public RemoteStoreShardShallowCopySnapshot(
      */
     public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser parser) throws IOException {
         String snapshot = null;
-        String version = DEFAULT_VERSION;
+        String version = null;
         long indexVersion = -1;
         long startTime = 0;
         long time = 0;
@@ -205,6 +251,7 @@ public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser pa
         }
 
         return new RemoteStoreShardShallowCopySnapshot(
+            version,
             snapshot,
             indexVersion,
             primaryTerm,
@@ -308,4 +355,43 @@ public long totalSize() {
         return totalSize;
     }
 
+    private void verifyParameters(
+        String version,
+        String snapshot,
+        long indexVersion,
+        long primaryTerm,
+        long commitGeneration,
+        String indexUUID,
+        String remoteStoreRepository,
+        String repositoryBasePath
+    ) {
+        String exceptionStr = null;
+        if (version == null) {
+            exceptionStr = "Invalid Version Provided";
+        }
+        if (snapshot == null) {
+            exceptionStr = "Invalid/Missing Snapshot Name";
+        }
+        if (indexVersion < 0) {
+            exceptionStr = "Invalid Index Version";
+        }
+        if (primaryTerm < 0) {
+            exceptionStr = "Invalid Primary Term";
+        }
+        if (commitGeneration < 0) {
+            exceptionStr = "Invalid Commit Generation";
+        }
+        if (indexUUID == null) {
+            exceptionStr = "Invalid/Missing Index UUID";
+        }
+        if (remoteStoreRepository == null) {
+            exceptionStr = "Invalid/Missing Remote Store Repository";
+        }
+        if (repositoryBasePath == null) {
+            exceptionStr = "Invalid/Missing Repository Base Path";
+        }
+        if (exceptionStr != null) {
+            throw new IllegalArgumentException(exceptionStr);
+        }
+    }
 }

server/src/main/java/org/opensearch/snapshots/SnapshotInfo.java

@@ -118,7 +118,7 @@ public static final class SnapshotInfoBuilder {
         private ShardStatsBuilder shardStatsBuilder = null;
         private Boolean includeGlobalState = null;
 
-        private Boolean remoteStoreIndexShallowCopy = false;
+        private Boolean remoteStoreIndexShallowCopy = null;
         private Map<String, Object> userMetadata = null;
         private int version = -1;
         private List<SnapshotShardFailure> shardFailures = null;
@@ -422,8 +422,6 @@ public SnapshotInfo(final StreamInput in) throws IOException {
         dataStreams = in.readStringList();
         if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
             remoteStoreIndexShallowCopy = in.readOptionalBoolean();
-        } else {
-            remoteStoreIndexShallowCopy = false;
         }
     }
 
@@ -747,7 +745,7 @@ public static SnapshotInfo fromXContentInternal(final XContentParser parser) thr
         int totalShards = 0;
         int successfulShards = 0;
         Boolean includeGlobalState = null;
-        Boolean remoteStoreIndexShallowCopy = false;
+        Boolean remoteStoreIndexShallowCopy = null;
         Map<String, Object> userMetadata = null;
         List<SnapshotShardFailure> shardFailures = Collections.emptyList();
         if (parser.currentToken() == null) { // fresh parser? move to the first token

server/src/main/java/org/opensearch/snapshots/SnapshotsService.java

@@ -338,9 +338,17 @@ public ClusterState execute(ClusterState currentState) {
                 }
 
                 boolean remoteStoreIndexShallowCopy = REMOTE_STORE_INDEX_SHALLOW_COPY.get(repository.getMetadata().settings());
-                if (remoteStoreIndexShallowCopy && !currentState.nodes().getMinNodeVersion().onOrAfter(Version.V_2_9_0)) {
+                Version minVersionInCluster = currentState.nodes().getMinNodeVersion();
+                if (remoteStoreIndexShallowCopy && !minVersionInCluster.onOrAfter(Version.V_2_9_0)) {
                     remoteStoreIndexShallowCopy = false;
-                    logger.warn("setting is ignored and will be used when all nodes are moved to version > 2.9");
+                    logger.warn(
+                        "setting "
+                            + REMOTE_STORE_INDEX_SHALLOW_COPY
+                            + " is ignored as some of the nodes in cluster have version < "
+                            + Version.V_2_9_0
+                            + ", minimum version found in cluster is "
+                            + minVersionInCluster
+                    );
                 }
                 newEntry = SnapshotsInProgress.startedEntry(
                     new Snapshot(repositoryName, snapshotId),

server/src/test/java/org/opensearch/index/snapshots/blobstore/RemoteStoreShardShallowCopySnapshotTests.java

@@ -135,9 +135,10 @@ public void testFromXContentInvalid() throws IOException {
             List<String> fileNames = new ArrayList<>(5);
             fileNames.addAll(Arrays.asList("file1", "file2", "file3", "file4", "file5"));
             String failure = null;
+            String version = RemoteStoreShardShallowCopySnapshot.DEFAULT_VERSION;
             long length = Math.max(0, Math.abs(randomLong()));
             // random corruption
-            switch (randomIntBetween(0, 7)) {
+            switch (randomIntBetween(0, 8)) {
                 case 0:
                     snapshot = null;
                     failure = "Invalid/Missing Snapshot Name";
@@ -167,13 +168,18 @@ public void testFromXContentInvalid() throws IOException {
                     failure = "Invalid/Missing Repository Base Path";
                     break;
                 case 7:
+                    version = null;
+                    failure = "Invalid Version Provided";
+                    break;
+                case 8:
                     break;
                 default:
                     fail("shouldn't be here");
             }
 
             XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON);
             builder.startObject();
+            builder.field(RemoteStoreShardShallowCopySnapshot.VERSION, version);
             builder.field(RemoteStoreShardShallowCopySnapshot.NAME, snapshot);
             builder.field(RemoteStoreShardShallowCopySnapshot.INDEX_VERSION, indexVersion);
             builder.field(RemoteStoreShardShallowCopySnapshot.START_TIME, startTime);
@@ -213,7 +219,7 @@ public void testFromXContentInvalid() throws IOException {
                     parser.nextToken();
                     RemoteStoreShardShallowCopySnapshot.fromXContent(parser);
                     fail("Should have failed with [" + failure + "]");
-                } catch (NullPointerException | AssertionError ex) {
+                } catch (IllegalArgumentException ex) {
                     assertThat(ex.getMessage(), containsString(failure));
                 }
             }

server/src/test/java/org/opensearch/repositories/blobstore/BlobStoreRepositoryTests.java

@@ -239,7 +239,7 @@ private String[] getLockFilesInRemoteStore(String remoteStoreIndex, String remot
         BlobPath shardLevelBlobPath = remoteStorerepository.basePath().add(indexUUID).add("0").add("segments").add("lock_files");
         BlobContainer blobContainer = remoteStorerepository.blobStore().blobContainer(shardLevelBlobPath);
         try (RemoteBufferedOutputDirectory lockDirectory = new RemoteBufferedOutputDirectory(blobContainer)) {
-            return lockDirectory.listAll();
+            return Arrays.stream(lockDirectory.listAll()).filter(lock -> lock.endsWith(".lock")).toArray(String[]::new);
         }
     }