Skip to content

Commit c98b8ac

Browse files
author
Harish Bhakuni
committed
Address PR Comments
Signed-off-by: Harish Bhakuni <hbhakuni@amazon.com>
1 parent 0beba99 commit c98b8ac

File tree

9 files changed

+35
-70
lines changed

9 files changed

+35
-70
lines changed

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

Lines changed: 17 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import java.io.IOException;
1818
import java.util.ArrayList;
1919
import java.util.List;
20+
import java.util.Objects;
2021

2122
/**
2223
* Remote Store based Shard snapshot metadata
@@ -26,6 +27,7 @@
2627
public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment {
2728

2829
private final String snapshot;
30+
private final String version;
2931
private final long indexVersion;
3032
private final long startTime;
3133
private final long time;
@@ -38,7 +40,9 @@ public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment {
3840
private final String indexUUID;
3941
private final List<String> fileNames;
4042

43+
private static final String DEFAULT_VERSION = "1";
4144
static final String NAME = "name";
45+
static final String VERSION = "version";
4246
static final String INDEX_VERSION = "index_version";
4347
static final String START_TIME = "start_time";
4448
static final String TIME = "time";
@@ -57,7 +61,7 @@ public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment {
5761
static final String TOTAL_SIZE = "total_size";
5862

5963
private static final ParseField PARSE_NAME = new ParseField(NAME);
60-
64+
private static final ParseField PARSE_VERSION = new ParseField(VERSION);
6165
private static final ParseField PARSE_PRIMARY_TERM = new ParseField(PRIMARY_TERM);
6266
private static final ParseField PARSE_COMMIT_GENERATION = new ParseField(COMMIT_GENERATION);
6367
private static final ParseField PARSE_INDEX_VERSION = new ParseField(INDEX_VERSION, "index-version");
@@ -78,6 +82,7 @@ public class RemoteStoreShardShallowCopySnapshot implements ToXContentFragment {
7882
*/
7983
@Override
8084
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
85+
builder.field(VERSION, version);
8186
builder.field(NAME, snapshot);
8287
builder.field(INDEX_VERSION, indexVersion);
8388
builder.field(START_TIME, startTime);
@@ -111,22 +116,22 @@ public RemoteStoreShardShallowCopySnapshot(
111116
String repositoryBasePath,
112117
List<String> fileNames
113118
) {
114-
assert snapshot != null;
115-
assert indexVersion >= 0;
116-
assert commitGeneration >= 0 && primaryTerm >= 0;
117-
assert indexUUID != null && remoteStoreRepository != null && repositoryBasePath != null;
118-
this.snapshot = snapshot;
119+
this.snapshot = Objects.requireNonNull(snapshot, "Invalid/Missing Snapshot Name");
120+
assert indexVersion > 0 : "Invalid Index Version";
121+
assert primaryTerm > 0 : "Invalid Primary Term";
122+
assert commitGeneration > 0 : "Invalid Commit Generation";
119123
this.indexVersion = indexVersion;
120124
this.primaryTerm = primaryTerm;
121125
this.commitGeneration = commitGeneration;
122126
this.startTime = startTime;
123127
this.time = time;
124128
this.totalFileCount = totalFileCount;
125129
this.totalSize = totalSize;
126-
this.indexUUID = indexUUID;
127-
this.remoteStoreRepository = remoteStoreRepository;
128-
this.repositoryBasePath = repositoryBasePath;
130+
this.indexUUID = Objects.requireNonNull(indexUUID, "Invalid/Missing Index UUID");
131+
this.remoteStoreRepository = Objects.requireNonNull(remoteStoreRepository, "Invalid/Missing Remote Store Repository");
132+
this.repositoryBasePath = Objects.requireNonNull(repositoryBasePath, "Invalid/Missing Repository Base Path");
129133
this.fileNames = fileNames;
134+
this.version = DEFAULT_VERSION;
130135
}
131136

132137
/**
@@ -137,6 +142,7 @@ public RemoteStoreShardShallowCopySnapshot(
137142
*/
138143
public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser parser) throws IOException {
139144
String snapshot = null;
145+
String version = DEFAULT_VERSION;
140146
long indexVersion = -1;
141147
long startTime = 0;
142148
long time = 0;
@@ -160,6 +166,8 @@ public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser pa
160166
} else if (token.isValue()) {
161167
if (PARSE_NAME.match(currentFieldName, parser.getDeprecationHandler())) {
162168
snapshot = parser.text();
169+
} else if (PARSE_VERSION.match(currentFieldName, parser.getDeprecationHandler())) {
170+
version = parser.text();
163171
} else if (PARSE_INDEX_VERSION.match(currentFieldName, parser.getDeprecationHandler())) {
164172
indexVersion = parser.longValue();
165173
} else if (PARSE_PRIMARY_TERM.match(currentFieldName, parser.getDeprecationHandler())) {
@@ -196,29 +204,6 @@ public static RemoteStoreShardShallowCopySnapshot fromXContent(XContentParser pa
196204
}
197205
}
198206

199-
// Verify that file information is complete
200-
if (snapshot == null) {
201-
throw new OpenSearchParseException("missing Snapshot Name");
202-
}
203-
if (indexVersion < 0) {
204-
throw new OpenSearchParseException("missing or invalid Index Version");
205-
}
206-
if (commitGeneration < 0) {
207-
throw new OpenSearchParseException("missing or invalid Commit Generation");
208-
}
209-
if (primaryTerm < 0) {
210-
throw new OpenSearchParseException("missing or invalid Primary Term");
211-
}
212-
if (indexUUID == null) {
213-
throw new OpenSearchParseException("missing Index UUID");
214-
}
215-
if (remoteStoreRepository == null) {
216-
throw new OpenSearchParseException("missing Remote Store Repository");
217-
}
218-
if (repositoryBasePath == null) {
219-
throw new OpenSearchParseException("missing Repository Base Path");
220-
}
221-
222207
return new RemoteStoreShardShallowCopySnapshot(
223208
snapshot,
224209
indexVersion,

‎server/src/main/java/org/opensearch/repositories/FilterRepository.java‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -185,28 +185,22 @@ public void snapshotShard(
185185
@Override
186186
public void snapshotRemoteStoreIndexShard(
187187
Store store,
188-
MapperService mapperService,
189188
SnapshotId snapshotId,
190189
IndexId indexId,
191190
IndexCommit snapshotIndexCommit,
192191
String shardStateIdentifier,
193192
IndexShardSnapshotStatus snapshotStatus,
194-
Version repositoryMetaVersion,
195-
Map<String, Object> userMetadata,
196193
long primaryTerm,
197194
long startTime,
198195
ActionListener<String> listener
199196
) {
200197
in.snapshotRemoteStoreIndexShard(
201198
store,
202-
mapperService,
203199
snapshotId,
204200
indexId,
205201
snapshotIndexCommit,
206202
shardStateIdentifier,
207203
snapshotStatus,
208-
repositoryMetaVersion,
209-
userMetadata,
210204
primaryTerm,
211205
startTime,
212206
listener

‎server/src/main/java/org/opensearch/repositories/Repository.java‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -259,30 +259,24 @@ void snapshotShard(
259259
* As snapshot process progresses, implementation of this method should update {@link IndexShardSnapshotStatus} object and check
260260
* {@link IndexShardSnapshotStatus#isAborted()} to see if the snapshot process should be aborted.
261261
* @param store store to be snapshotted
262-
* @param mapperService the shards mapper service
263262
* @param snapshotId snapshot id
264263
* @param indexId id for the index being snapshotted
265264
* @param snapshotIndexCommit commit point
266265
* @param shardStateIdentifier a unique identifier of the state of the shard that is stored with the shard's snapshot and used
267266
* to detect if the shard has changed between snapshots. If {@code null} is passed as the identifier
268267
* snapshotting will be done by inspecting the physical files referenced by {@code snapshotIndexCommit}
269268
* @param snapshotStatus snapshot status
270-
* @param repositoryMetaVersion version of the updated repository metadata to write
271-
* @param userMetadata user metadata of the snapshot found in {@link SnapshotsInProgress.Entry#userMetadata()}
272269
* @param primaryTerm current Primary Term
273270
* @param startTime start time of the snapshot commit, this will be used as the start time for snapshot.
274271
* @param listener listener invoked on completion
275272
*/
276273
default void snapshotRemoteStoreIndexShard(
277274
Store store,
278-
MapperService mapperService,
279275
SnapshotId snapshotId,
280276
IndexId indexId,
281277
IndexCommit snapshotIndexCommit,
282278
@Nullable String shardStateIdentifier,
283279
IndexShardSnapshotStatus snapshotStatus,
284-
Version repositoryMetaVersion,
285-
Map<String, Object> userMetadata,
286280
long primaryTerm,
287281
long startTime,
288282
ActionListener<String> listener

‎server/src/main/java/org/opensearch/repositories/blobstore/BlobStoreRepository.java‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2303,14 +2303,11 @@ private void writeAtomic(BlobContainer container, final String blobName, final B
23032303
@Override
23042304
public void snapshotRemoteStoreIndexShard(
23052305
Store store,
2306-
MapperService mapperService,
23072306
SnapshotId snapshotId,
23082307
IndexId indexId,
23092308
IndexCommit snapshotIndexCommit,
23102309
String shardStateIdentifier,
23112310
IndexShardSnapshotStatus snapshotStatus,
2312-
Version repositoryMetaVersion,
2313-
Map<String, Object> userMetadata,
23142311
long primaryTerm,
23152312
long startTime,
23162313
ActionListener<String> listener
@@ -2365,7 +2362,7 @@ public void snapshotRemoteStoreIndexShard(
23652362
),
23662363
shardContainer,
23672364
snapshotId.getUUID(),
2368-
compress
2365+
compressor
23692366
);
23702367
} catch (IOException e) {
23712368
throw new IndexShardSnapshotFailedException(

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,8 @@ public SnapshotInfo(final StreamInput in) throws IOException {
422422
dataStreams = in.readStringList();
423423
if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
424424
remoteStoreIndexShallowCopy = in.readOptionalBoolean();
425+
} else {
426+
remoteStoreIndexShallowCopy = false;
425427
}
426428
}
427429

@@ -745,7 +747,7 @@ public static SnapshotInfo fromXContentInternal(final XContentParser parser) thr
745747
int totalShards = 0;
746748
int successfulShards = 0;
747749
Boolean includeGlobalState = null;
748-
Boolean remoteStoreIndexShallowCopy = null;
750+
Boolean remoteStoreIndexShallowCopy = false;
749751
Map<String, Object> userMetadata = null;
750752
List<SnapshotShardFailure> shardFailures = Collections.emptyList();
751753
if (parser.currentToken() == null) { // fresh parser? move to the first token

‎server/src/main/java/org/opensearch/snapshots/SnapshotShardsService.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -404,14 +404,11 @@ private void snapshot(
404404
try {
405405
repository.snapshotRemoteStoreIndexShard(
406406
indexShard.store(),
407-
indexShard.mapperService(),
408407
snapshot.getSnapshotId(),
409408
indexId,
410409
wrappedSnapshot.get(),
411410
getShardStateId(indexShard, snapshotIndexCommit),
412411
snapshotStatus,
413-
version,
414-
userMetadata,
415412
primaryTerm,
416413
startTime,
417414
ActionListener.runBefore(listener, wrappedSnapshot::close)

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -337,10 +337,10 @@ public ClusterState execute(ClusterState currentState) {
337337
}
338338
}
339339

340-
boolean remoteStoreIndexShallowCopy = false;
341-
if (version.onOrAfter(Version.V_2_8_0)) {
342-
// we will pull the remote store shallow copy flag value from repository settings.
343-
remoteStoreIndexShallowCopy = REMOTE_STORE_INDEX_SHALLOW_COPY.get(repository.getMetadata().settings());
340+
boolean remoteStoreIndexShallowCopy = REMOTE_STORE_INDEX_SHALLOW_COPY.get(repository.getMetadata().settings());
341+
if (remoteStoreIndexShallowCopy && !currentState.nodes().getMinNodeVersion().onOrAfter(Version.V_2_9_0)) {
342+
remoteStoreIndexShallowCopy = false;
343+
logger.warn("setting is ignored and will be used when all nodes are moved to version > 2.9");
344344
}
345345
newEntry = SnapshotsInProgress.startedEntry(
346346
new Snapshot(repositoryName, snapshotId),

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

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
package org.opensearch.index.snapshots.blobstore;
1010

11-
import org.opensearch.OpenSearchParseException;
1211
import org.opensearch.common.Strings;
1312
import org.opensearch.common.bytes.BytesReference;
1413
import org.opensearch.common.xcontent.XContentFactory;
@@ -64,7 +63,7 @@ public void testToXContent() throws IOException {
6463
builder.endObject();
6564
actual = Strings.toString(builder);
6665
}
67-
String expectedXContent = "{\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123,"
66+
String expectedXContent = "{\"version\":\"1\",\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123,"
6867
+ "\"number_of_files\":5,\"total_size\":5,\"index_uuid\":\"syzhajds-ashdlfj\",\"remote_store_repository\":"
6968
+ "\"test-rs-repository\",\"commit_generation\":5,\"primary_term\":3,\"remote_store_repository_base_path\":"
7069
+ "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"]}";
@@ -99,7 +98,7 @@ public void testFromXContent() throws IOException {
9998
repositoryBasePath,
10099
fileNames
101100
);
102-
String xContent = "{\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123,"
101+
String xContent = "{\"version\":\"1\",\"name\":\"test-snapshot\",\"index_version\":1,\"start_time\":123,\"time\":123,"
103102
+ "\"number_of_files\":5,\"total_size\":5,\"index_uuid\":\"syzhajds-ashdlfj\",\"remote_store_repository\":"
104103
+ "\"test-rs-repository\",\"commit_generation\":5,\"primary_term\":3,\"remote_store_repository_base_path\":"
105104
+ "\"test-repo-basepath\",\"file_names\":[\"file1\",\"file2\",\"file3\",\"file4\",\"file5\"]}";
@@ -141,31 +140,31 @@ public void testFromXContentInvalid() throws IOException {
141140
switch (randomIntBetween(0, 7)) {
142141
case 0:
143142
snapshot = null;
144-
failure = "missing Snapshot Name";
143+
failure = "Invalid/Missing Snapshot Name";
145144
break;
146145
case 1:
147146
indexVersion = -Math.abs(randomLong());
148-
failure = "missing or invalid Index Version";
147+
failure = "Invalid Index Version";
149148
break;
150149
case 2:
151150
commitGeneration = -Math.abs(randomLong());
152-
failure = "missing or invalid Commit Generation";
151+
failure = "Invalid Commit Generation";
153152
break;
154153
case 3:
155154
primaryTerm = -Math.abs(randomLong());
156-
failure = "missing or invalid Primary Term";
155+
failure = "Invalid Primary Term";
157156
break;
158157
case 4:
159158
indexUUID = null;
160-
failure = "missing Index UUID";
159+
failure = "Invalid/Missing Index UUID";
161160
break;
162161
case 5:
163162
remoteStoreRepository = null;
164-
failure = "missing Remote Store Repository";
163+
failure = "Invalid/Missing Remote Store Repository";
165164
break;
166165
case 6:
167166
repositoryBasePath = null;
168-
failure = "missing Repository Base Path";
167+
failure = "Invalid/Missing Repository Base Path";
169168
break;
170169
case 7:
171170
break;
@@ -214,7 +213,7 @@ public void testFromXContentInvalid() throws IOException {
214213
parser.nextToken();
215214
RemoteStoreShardShallowCopySnapshot.fromXContent(parser);
216215
fail("Should have failed with [" + failure + "]");
217-
} catch (OpenSearchParseException ex) {
216+
} catch (NullPointerException | AssertionError ex) {
218217
assertThat(ex.getMessage(), containsString(failure));
219218
}
220219
}

‎server/src/test/java/org/opensearch/repositories/RepositoriesServiceTests.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -318,14 +318,11 @@ public void snapshotShard(
318318
@Override
319319
public void snapshotRemoteStoreIndexShard(
320320
Store store,
321-
MapperService mapperService,
322321
SnapshotId snapshotId,
323322
IndexId indexId,
324323
IndexCommit snapshotIndexCommit,
325324
String shardStateIdentifier,
326325
IndexShardSnapshotStatus snapshotStatus,
327-
Version repositoryMetaVersion,
328-
Map<String, Object> userMetadata,
329326
long primaryTerm,
330327
long startTime,
331328
ActionListener<String> listener

0 commit comments

Comments
 (0)