Skip to content

Commit 474b486

Browse files
ywangdywangd
committed
address review comments
1 parent 4656baa commit 474b486

File tree

5 files changed

+52
-53
lines changed

5 files changed

+52
-53
lines changed

‎server/src/main/java/org/elasticsearch/cluster/SnapshotsInProgress.java‎

Lines changed: 29 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.elasticsearch.TransportVersion;
1313
import org.elasticsearch.TransportVersions;
1414
import org.elasticsearch.cluster.ClusterState.Custom;
15+
import org.elasticsearch.cluster.metadata.Metadata;
1516
import org.elasticsearch.cluster.metadata.ProjectId;
1617
import org.elasticsearch.cluster.metadata.SingleNodeShutdownMetadata;
1718
import org.elasticsearch.cluster.node.DiscoveryNode;
@@ -23,6 +24,7 @@
2324
import org.elasticsearch.common.util.Maps;
2425
import org.elasticsearch.common.util.set.Sets;
2526
import org.elasticsearch.common.xcontent.ChunkedToXContentHelper;
27+
import org.elasticsearch.core.FixForMultiProject;
2628
import org.elasticsearch.core.Nullable;
2729
import org.elasticsearch.core.SuppressForbidden;
2830
import org.elasticsearch.core.Tuple;
@@ -59,7 +61,6 @@
5961
import java.util.stream.Stream;
6062

6163
import static org.elasticsearch.repositories.RepositoryOperation.PROJECT_REPO_SERIALIZER;
62-
import static org.elasticsearch.repositories.RepositoryOperation.defaultProjectRepo;
6364

6465
/**
6566
* Meta data about snapshots that are currently executing
@@ -132,16 +133,18 @@ private SnapshotsInProgress(Map<ProjectRepo, ByRepo> entries, Set<String> nodesI
132133
assert assertConsistentEntries(this.entries);
133134
}
134135

136+
@FixForMultiProject
135137
@Deprecated(forRemoval = true)
136138
public SnapshotsInProgress withUpdatedEntriesForRepo(String repository, List<Entry> updatedEntries) {
137-
return withUpdatedEntriesForRepo(defaultProjectRepo(repository), updatedEntries);
139+
return withUpdatedEntriesForRepo(Metadata.DEFAULT_PROJECT_ID, repository, updatedEntries);
138140
}
139141

140-
public SnapshotsInProgress withUpdatedEntriesForRepo(ProjectRepo projectRepo, List<Entry> updatedEntries) {
141-
if (updatedEntries.equals(forRepo(projectRepo))) {
142+
public SnapshotsInProgress withUpdatedEntriesForRepo(ProjectId projectId, String repository, List<Entry> updatedEntries) {
143+
if (updatedEntries.equals(forRepo(projectId, repository))) {
142144
return this;
143145
}
144146
final Map<ProjectRepo, ByRepo> copy = new HashMap<>(this.entries);
147+
final var projectRepo = new ProjectRepo(projectId, repository);
145148
if (updatedEntries.isEmpty()) {
146149
copy.remove(projectRepo);
147150
if (copy.isEmpty()) {
@@ -154,24 +157,25 @@ public SnapshotsInProgress withUpdatedEntriesForRepo(ProjectRepo projectRepo, Li
154157
}
155158

156159
public SnapshotsInProgress withAddedEntry(Entry entry) {
157-
final List<Entry> forRepo = new ArrayList<>(forRepo(entry.projectRepo()));
160+
final List<Entry> forRepo = new ArrayList<>(forRepo(entry.projectId(), entry.repository()));
158161
forRepo.add(entry);
159-
return withUpdatedEntriesForRepo(entry.projectRepo(), forRepo);
162+
return withUpdatedEntriesForRepo(entry.projectId(), entry.repository(), forRepo);
160163
}
161164

162165
/**
163166
* Returns the list of snapshots in the specified repository.
164167
*/
168+
@FixForMultiProject
165169
@Deprecated(forRemoval = true)
166170
public List<Entry> forRepo(String repository) {
167-
return entries.getOrDefault(defaultProjectRepo(repository), ByRepo.EMPTY).entries;
171+
return forRepo(Metadata.DEFAULT_PROJECT_ID, repository);
168172
}
169173

170174
/**
171175
* Returns the list of snapshots in the specified repository.
172176
*/
173-
public List<Entry> forRepo(ProjectRepo projectRepo) {
174-
return entries.getOrDefault(projectRepo, ByRepo.EMPTY).entries;
177+
public List<Entry> forRepo(ProjectId projectId, String repository) {
178+
return entries.getOrDefault(new ProjectRepo(projectId, repository), ByRepo.EMPTY).entries;
175179
}
176180

177181
public boolean isEmpty() {
@@ -196,7 +200,7 @@ public Stream<Entry> asStream() {
196200

197201
@Nullable
198202
public Entry snapshot(final Snapshot snapshot) {
199-
return findSnapshotInList(snapshot, forRepo(snapshot.getProjectRepo()));
203+
return findSnapshotInList(snapshot, forRepo(snapshot.getProjectId(), snapshot.getRepository()));
200204
}
201205

202206
/**
@@ -224,12 +228,13 @@ private static Entry findSnapshotInList(Snapshot snapshotToFind, List<Entry> for
224228
* in-progress shard snapshots that were not yet finalized when it began. All these other in-progress shard snapshot lists are scheduled
225229
* for deletion now.
226230
*/
231+
@FixForMultiProject
227232
@Deprecated(forRemoval = true)
228233
public Map<RepositoryShardId, Set<ShardGeneration>> obsoleteGenerations(
229234
String repository,
230235
SnapshotsInProgress oldClusterStateSnapshots
231236
) {
232-
return obsoleteGenerations(defaultProjectRepo(repository), oldClusterStateSnapshots);
237+
return obsoleteGenerations(Metadata.DEFAULT_PROJECT_ID, repository, oldClusterStateSnapshots);
233238
}
234239

235240
/**
@@ -243,13 +248,14 @@ public Map<RepositoryShardId, Set<ShardGeneration>> obsoleteGenerations(
243248
* for deletion now.
244249
*/
245250
public Map<RepositoryShardId, Set<ShardGeneration>> obsoleteGenerations(
246-
ProjectRepo projectRepo,
251+
ProjectId projectId,
252+
String repository,
247253
SnapshotsInProgress oldClusterStateSnapshots
248254
) {
249255
final Map<RepositoryShardId, Set<ShardGeneration>> obsoleteGenerations = new HashMap<>();
250-
final List<Entry> latestSnapshots = forRepo(projectRepo);
256+
final List<Entry> latestSnapshots = forRepo(projectId, repository);
251257

252-
for (Entry oldEntry : oldClusterStateSnapshots.forRepo(projectRepo)) {
258+
for (Entry oldEntry : oldClusterStateSnapshots.forRepo(projectId, repository)) {
253259
final Entry matchingLatestEntry = findSnapshotInList(oldEntry.snapshot(), latestSnapshots);
254260
if (matchingLatestEntry == null || matchingLatestEntry == oldEntry) {
255261
// The snapshot progress has not changed.
@@ -456,7 +462,8 @@ private static boolean assertConsistentEntries(Map<ProjectRepo, ByRepo> entries)
456462
final ProjectRepo repository = repoEntries.getKey();
457463
assert entriesForRepository.isEmpty() == false : "found empty list of snapshots for " + repository + " in " + entries;
458464
for (Entry entry : entriesForRepository) {
459-
assert entry.projectRepo().equals(repository) : "mismatched repository " + entry + " tracked under " + repository;
465+
assert new ProjectRepo(entry.projectId(), entry.repository()).equals(repository)
466+
: "mismatched repository " + entry + " tracked under " + repository;
460467
for (Map.Entry<RepositoryShardId, ShardSnapshotStatus> shard : entry.shardSnapshotStatusByRepoShardId().entrySet()) {
461468
final RepositoryShardId sid = shard.getKey();
462469
final ShardSnapshotStatus shardSnapshotStatus = shard.getValue();
@@ -1287,10 +1294,6 @@ public String repository() {
12871294
return snapshot.getRepository();
12881295
}
12891296

1290-
public ProjectRepo projectRepo() {
1291-
return new ProjectRepo(projectId(), repository());
1292-
}
1293-
12941297
public Snapshot snapshot() {
12951298
return this.snapshot;
12961299
}
@@ -1793,7 +1796,7 @@ private static final class SnapshotInProgressDiff implements NamedDiff<Custom> {
17931796
);
17941797
this.mapDiff = DiffableUtils.jdkMapDiffWithUpdatedKeys(
17951798
oldMapDiff,
1796-
RepositoryOperation::defaultProjectRepo,
1799+
repository -> new ProjectRepo(ProjectId.DEFAULT, repository),
17971800
PROJECT_REPO_SERIALIZER
17981801
);
17991802
} else {
@@ -1835,12 +1838,12 @@ public void writeTo(StreamOutput out) throws IOException {
18351838
if (out.getTransportVersion().before(TransportVersions.PROJECT_ID_IN_SNAPSHOT)) {
18361839
DiffableUtils.jdkMapDiffWithUpdatedKeys(mapDiff, projectRepo -> {
18371840
if (ProjectId.DEFAULT.equals(projectRepo.projectId()) == false) {
1838-
throw new IllegalArgumentException(
1839-
"Cannot write instance with non-default project id "
1840-
+ projectRepo.projectId()
1841-
+ " to version before "
1842-
+ TransportVersions.PROJECT_ID_IN_SNAPSHOT
1843-
);
1841+
final var message = "Cannot write instance with non-default project id "
1842+
+ projectRepo.projectId()
1843+
+ " to version before "
1844+
+ TransportVersions.PROJECT_ID_IN_SNAPSHOT;
1845+
assert false : message;
1846+
throw new IllegalArgumentException(message);
18441847
}
18451848
return projectRepo.repoName();
18461849
}, DiffableUtils.getStringKeySerializer()).writeTo(out);

‎server/src/main/java/org/elasticsearch/repositories/RepositoryOperation.java‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
package org.elasticsearch.repositories;
1010

1111
import org.elasticsearch.cluster.DiffableUtils;
12+
import org.elasticsearch.cluster.metadata.Metadata;
1213
import org.elasticsearch.cluster.metadata.ProjectId;
1314
import org.elasticsearch.common.io.stream.StreamInput;
1415
import org.elasticsearch.common.io.stream.StreamOutput;
@@ -27,23 +28,24 @@ public interface RepositoryOperation {
2728
*/
2829
@FixForMultiProject(description = "default implementation is temporary")
2930
default ProjectId projectId() {
30-
return ProjectId.DEFAULT;
31+
return Metadata.DEFAULT_PROJECT_ID;
3132
}
3233

3334
/**
3435
* Name of the repository affected.
3536
*/
3637
String repository();
3738

38-
default ProjectRepo projectRepo() {
39-
return new ProjectRepo(projectId(), repository());
40-
}
41-
4239
/**
4340
* The repository state id at the time the operation began.
4441
*/
4542
long repositoryStateId();
4643

44+
/**
45+
* A project qualified repository
46+
* @param projectId The project that the repository belongs to
47+
* @param repoName Name of the repository
48+
*/
4749
record ProjectRepo(ProjectId projectId, String repoName) implements Writeable {
4850

4951
public ProjectRepo(StreamInput in) throws IOException {
@@ -68,9 +70,4 @@ public ProjectRepo readKey(StreamInput in) throws IOException {
6870
return new ProjectRepo(in);
6971
}
7072
};
71-
72-
@Deprecated(forRemoval = true)
73-
static ProjectRepo defaultProjectRepo(String repoName) {
74-
return new ProjectRepo(ProjectId.DEFAULT, repoName);
75-
}
7673
}

‎server/src/main/java/org/elasticsearch/snapshots/InFlightShardSnapshotStates.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import org.elasticsearch.cluster.SnapshotsInProgress;
1313
import org.elasticsearch.core.Nullable;
1414
import org.elasticsearch.repositories.IndexId;
15+
import org.elasticsearch.repositories.RepositoryOperation.ProjectRepo;
1516
import org.elasticsearch.repositories.RepositoryShardId;
1617
import org.elasticsearch.repositories.ShardGeneration;
1718
import org.elasticsearch.repositories.ShardGenerations;
@@ -46,7 +47,7 @@ public static InFlightShardSnapshotStates forEntries(List<SnapshotsInProgress.En
4647
}
4748
final Map<String, Map<Integer, ShardGeneration>> generations = new HashMap<>();
4849
final Map<String, Set<Integer>> busyIds = new HashMap<>();
49-
assert snapshots.stream().map(SnapshotsInProgress.Entry::projectRepo).distinct().count() == 1
50+
assert snapshots.stream().map(entry -> new ProjectRepo(entry.projectId(), entry.repository())).distinct().count() == 1
5051
: "snapshots must either be an empty list or all belong to the same repository but saw " + snapshots;
5152
for (SnapshotsInProgress.Entry runningSnapshot : snapshots) {
5253
for (Map.Entry<RepositoryShardId, SnapshotsInProgress.ShardSnapshotStatus> shard : runningSnapshot

‎server/src/main/java/org/elasticsearch/snapshots/Snapshot.java‎

Lines changed: 6 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,6 @@
1818
import java.io.IOException;
1919
import java.util.Objects;
2020

21-
import static org.elasticsearch.repositories.RepositoryOperation.ProjectRepo;
22-
2321
/**
2422
* Basic information about a snapshot - a SnapshotId and the repository that the snapshot belongs to.
2523
*/
@@ -73,10 +71,6 @@ public String getRepository() {
7371
return repository;
7472
}
7573

76-
public ProjectRepo getProjectRepo() {
77-
return new ProjectRepo(projectId, repository);
78-
}
79-
8074
/**
8175
* Gets the snapshot id for the snapshot.
8276
*/
@@ -114,12 +108,12 @@ private int computeHashCode() {
114108
public void writeTo(final StreamOutput out) throws IOException {
115109
if (out.getTransportVersion().before(TransportVersions.PROJECT_ID_IN_SNAPSHOT)) {
116110
if (ProjectId.DEFAULT.equals(projectId) == false) {
117-
throw new IllegalArgumentException(
118-
"Cannot write instance with non-default project id "
119-
+ projectId
120-
+ " to version before "
121-
+ TransportVersions.PROJECT_ID_IN_SNAPSHOT
122-
);
111+
final var message = "Cannot write instance with non-default project id "
112+
+ projectId
113+
+ " to version before "
114+
+ TransportVersions.PROJECT_ID_IN_SNAPSHOT;
115+
assert false : message;
116+
throw new IllegalArgumentException(message);
123117
}
124118
} else {
125119
projectId.writeTo(out);

‎server/src/test/java/org/elasticsearch/snapshots/SnapshotsInProgressSerializationTests.java‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.elasticsearch.index.IndexVersion;
3636
import org.elasticsearch.index.shard.ShardId;
3737
import org.elasticsearch.repositories.IndexId;
38+
import org.elasticsearch.repositories.RepositoryOperation.ProjectRepo;
3839
import org.elasticsearch.repositories.ShardGeneration;
3940
import org.elasticsearch.repositories.ShardSnapshotResult;
4041
import org.elasticsearch.test.AbstractChunkedSerializingTestCase;
@@ -246,7 +247,8 @@ protected Custom makeTestChanges(Custom testInstance, Supplier<Entry> randomEntr
246247
if (randomBoolean()) {
247248
entries = shuffledList(entries);
248249
}
249-
updatedInstance = updatedInstance.withUpdatedEntriesForRepo(perRepoEntries.get(0).projectRepo(), entries);
250+
final Entry firstEntry = perRepoEntries.get(0);
251+
updatedInstance = updatedInstance.withUpdatedEntriesForRepo(firstEntry.projectId(), firstEntry.repository(), entries);
250252
}
251253
}
252254
return updatedInstance;
@@ -272,9 +274,11 @@ protected Custom mutateInstance(Custom instance) {
272274
} else {
273275
// mutate or remove an entry
274276
final var repo = randomFrom(
275-
snapshotsInProgress.asStream().map(SnapshotsInProgress.Entry::projectRepo).collect(Collectors.toSet())
277+
snapshotsInProgress.asStream()
278+
.map(entry -> new ProjectRepo(entry.projectId(), entry.repository()))
279+
.collect(Collectors.toSet())
276280
);
277-
final List<Entry> forRepo = snapshotsInProgress.forRepo(repo);
281+
final List<Entry> forRepo = snapshotsInProgress.forRepo(repo.projectId(), repo.repoName());
278282
int index = randomIntBetween(0, forRepo.size() - 1);
279283
Entry entry = forRepo.get(index);
280284
final List<Entry> updatedEntries = new ArrayList<>(forRepo);
@@ -283,7 +287,7 @@ protected Custom mutateInstance(Custom instance) {
283287
} else {
284288
updatedEntries.remove(index);
285289
}
286-
return snapshotsInProgress.withUpdatedEntriesForRepo(repo, updatedEntries);
290+
return snapshotsInProgress.withUpdatedEntriesForRepo(repo.projectId(), repo.repoName(), updatedEntries);
287291
}
288292
} else {
289293
return snapshotsInProgress.withUpdatedNodeIdsForRemoval(

0 commit comments

Comments
 (0)