-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Record project deletions in ProjectStateRegistry #130225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6a296e3
c33e116
6329301
fb0bb16
d5d2c6a
c5f2a3b
de4de95
2fa3a84
bd57bde
1b724f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,28 +20,49 @@ | |
import org.elasticsearch.common.io.stream.StreamInput; | ||
import org.elasticsearch.common.io.stream.StreamOutput; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.common.util.set.Sets; | ||
import org.elasticsearch.xcontent.ToXContent; | ||
|
||
import java.io.IOException; | ||
import java.util.Collections; | ||
import java.util.HashSet; | ||
import java.util.Iterator; | ||
import java.util.Map; | ||
import java.util.Objects; | ||
import java.util.Set; | ||
|
||
/** | ||
* Represents a registry for managing and retrieving project-specific state in the cluster state. | ||
*/ | ||
public class ProjectStateRegistry extends AbstractNamedDiffable<ClusterState.Custom> implements ClusterState.Custom { | ||
public static final String TYPE = "projects_registry"; | ||
public static final ProjectStateRegistry EMPTY = new ProjectStateRegistry(Collections.emptyMap()); | ||
public static final ProjectStateRegistry EMPTY = new ProjectStateRegistry(Collections.emptyMap(), Collections.emptySet(), 0); | ||
|
||
private final Map<ProjectId, Settings> projectsSettings; | ||
// Projects that have been marked for deletion based on their file-based setting | ||
private final Set<ProjectId> projectsMarkedForDeletion; | ||
// A counter that is incremented each time one or more projects are marked for deletion. | ||
private final long projectsMarkedForDeletionGeneration; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is very similar to |
||
|
||
public ProjectStateRegistry(StreamInput in) throws IOException { | ||
projectsSettings = in.readMap(ProjectId::readFrom, Settings::readSettingsFromStream); | ||
if (in.getTransportVersion().onOrAfter(TransportVersions.PROJECT_STATE_REGISTRY_RECORDS_DELETIONS)) { | ||
projectsMarkedForDeletion = in.readCollectionAsImmutableSet(ProjectId::readFrom); | ||
projectsMarkedForDeletionGeneration = in.readVLong(); | ||
} else { | ||
projectsMarkedForDeletion = Collections.emptySet(); | ||
projectsMarkedForDeletionGeneration = 0; | ||
} | ||
} | ||
|
||
private ProjectStateRegistry(Map<ProjectId, Settings> projectsSettings) { | ||
private ProjectStateRegistry( | ||
Map<ProjectId, Settings> projectsSettings, | ||
Set<ProjectId> projectsMarkedForDeletion, | ||
long projectsMarkedForDeletionGeneration | ||
) { | ||
this.projectsSettings = projectsSettings; | ||
this.projectsMarkedForDeletion = projectsMarkedForDeletion; | ||
this.projectsMarkedForDeletionGeneration = projectsMarkedForDeletionGeneration; | ||
} | ||
|
||
/** | ||
|
@@ -72,9 +93,11 @@ public Iterator<? extends ToXContent> toXContentChunked(ToXContent.Params params | |
builder.startObject("settings"); | ||
entry.getValue().toXContent(builder, new ToXContent.MapParams(Collections.singletonMap("flat_settings", "true"))); | ||
builder.endObject(); | ||
builder.field("marked_for_deletion", projectsMarkedForDeletion.contains(entry.getKey())); | ||
return builder.endObject(); | ||
}), | ||
Iterators.single((builder, p) -> builder.endArray()) | ||
Iterators.single((builder, p) -> builder.endArray()), | ||
Iterators.single((builder, p) -> builder.field("projects_marked_for_deletion_generation", projectsMarkedForDeletionGeneration)) | ||
); | ||
} | ||
|
||
|
@@ -95,12 +118,44 @@ public TransportVersion getMinimalSupportedVersion() { | |
@Override | ||
public void writeTo(StreamOutput out) throws IOException { | ||
out.writeMap(projectsSettings); | ||
if (out.getTransportVersion().onOrAfter(TransportVersions.PROJECT_STATE_REGISTRY_RECORDS_DELETIONS)) { | ||
out.writeCollection(projectsMarkedForDeletion); | ||
out.writeVLong(projectsMarkedForDeletionGeneration); | ||
} else { | ||
// There should be no deletion unless all MP nodes are at or after PROJECT_STATE_REGISTRY_RECORDS_DELETIONS | ||
assert projectsMarkedForDeletion.isEmpty(); | ||
assert projectsMarkedForDeletionGeneration == 0; | ||
} | ||
} | ||
|
||
public int size() { | ||
return projectsSettings.size(); | ||
} | ||
|
||
public long getProjectsMarkedForDeletionGeneration() { | ||
return projectsMarkedForDeletionGeneration; | ||
} | ||
|
||
// visible for testing | ||
Map<ProjectId, Settings> getProjectsSettings() { | ||
return Collections.unmodifiableMap(projectsSettings); | ||
} | ||
|
||
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) return true; | ||
if (o instanceof ProjectStateRegistry == false) return false; | ||
ProjectStateRegistry that = (ProjectStateRegistry) o; | ||
return projectsMarkedForDeletionGeneration == that.projectsMarkedForDeletionGeneration | ||
&& Objects.equals(projectsSettings, that.projectsSettings) | ||
&& Objects.equals(projectsMarkedForDeletion, that.projectsMarkedForDeletion); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(projectsSettings, projectsMarkedForDeletion, projectsMarkedForDeletionGeneration); | ||
} | ||
Comment on lines
+144
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Many of the objects in cluster state compares equality by identity, e.g. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasted a bit of time trying to make it happen before realizing how ugly it can get since this is a bit more than ClusterBlocks and would require more hacking. In fact, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (same with HealthMetadata, RestoreInProgress, to name a few other customs) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. There is indeed inconsistency in how we handle equals for cluster state objects. Since this object is linear to number of projects, the upper-bound should not be too high for performing a non-identity equals. We can also revisit it if it proves to be a problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we make this class support the more granular diff than the generic CompleteNameDiff. The identity equality will be much less interesting since I think creating diff is one place |
||
|
||
public static Builder builder(ClusterState original) { | ||
ProjectStateRegistry projectRegistry = original.custom(TYPE, EMPTY); | ||
return builder(projectRegistry); | ||
|
@@ -116,22 +171,46 @@ public static Builder builder() { | |
|
||
public static class Builder { | ||
private final ImmutableOpenMap.Builder<ProjectId, Settings> projectsSettings; | ||
private final Set<ProjectId> projectsMarkedForDeletion; | ||
private final long projectsMarkedForDeletionGeneration; | ||
private boolean newProjectMarkedForDeletion = false; | ||
|
||
private Builder() { | ||
this.projectsSettings = ImmutableOpenMap.builder(); | ||
projectsMarkedForDeletion = new HashSet<>(); | ||
projectsMarkedForDeletionGeneration = 0; | ||
} | ||
|
||
private Builder(ProjectStateRegistry original) { | ||
this.projectsSettings = ImmutableOpenMap.builder(original.projectsSettings); | ||
this.projectsMarkedForDeletion = new HashSet<>(original.projectsMarkedForDeletion); | ||
this.projectsMarkedForDeletionGeneration = original.projectsMarkedForDeletionGeneration; | ||
} | ||
|
||
public Builder putProjectSettings(ProjectId projectId, Settings settings) { | ||
projectsSettings.put(projectId, settings); | ||
return this; | ||
} | ||
|
||
public Builder markProjectForDeletion(ProjectId projectId) { | ||
if (projectsMarkedForDeletion.add(projectId)) { | ||
newProjectMarkedForDeletion = true; | ||
} | ||
return this; | ||
} | ||
|
||
public ProjectStateRegistry build() { | ||
return new ProjectStateRegistry(projectsSettings.build()); | ||
final var unknownButUnderDeletion = Sets.difference(projectsMarkedForDeletion, projectsSettings.keys()); | ||
if (unknownButUnderDeletion.isEmpty() == false) { | ||
throw new IllegalArgumentException( | ||
"Cannot mark projects for deletion that are not in the registry: " + unknownButUnderDeletion | ||
); | ||
ywangd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return new ProjectStateRegistry( | ||
projectsSettings.build(), | ||
projectsMarkedForDeletion, | ||
newProjectMarkedForDeletion ? projectsMarkedForDeletionGeneration + 1 : projectsMarkedForDeletionGeneration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to reset this generation when cluster term changes similar to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have to. AFAICT, with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK I also think it's safe to not reset. Overflow does not seem to be a conern either. I do wonder why we reset There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. conceptually it makes sense there and it is possible to do it easier (code-wise) underneath when we build a DiscoveryNodes. If it is not needed, convenient to do, and it makes sense only within a term, why not reset it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (that's how I see it at least after staring at it for a while!) |
||
); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
package org.elasticsearch.cluster.project; | ||
|
||
import org.elasticsearch.cluster.ClusterModule; | ||
import org.elasticsearch.cluster.ClusterState; | ||
import org.elasticsearch.cluster.Diff; | ||
import org.elasticsearch.common.io.stream.NamedWriteableRegistry; | ||
import org.elasticsearch.common.io.stream.Writeable; | ||
import org.elasticsearch.common.settings.Settings; | ||
import org.elasticsearch.test.ESTestCase; | ||
import org.elasticsearch.test.SimpleDiffableWireSerializationTestCase; | ||
|
||
import java.io.IOException; | ||
import java.util.stream.IntStream; | ||
|
||
public class ProjectStateRegistrySerializationTests extends SimpleDiffableWireSerializationTestCase<ClusterState.Custom> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was no serialization test for this, so I added this. |
||
|
||
@Override | ||
protected ClusterState.Custom makeTestChanges(ClusterState.Custom testInstance) { | ||
return mutate((ProjectStateRegistry) testInstance); | ||
} | ||
|
||
@Override | ||
protected Writeable.Reader<Diff<ClusterState.Custom>> diffReader() { | ||
return ProjectStateRegistry::readDiffFrom; | ||
} | ||
|
||
@Override | ||
protected NamedWriteableRegistry getNamedWriteableRegistry() { | ||
return new NamedWriteableRegistry(ClusterModule.getNamedWriteables()); | ||
} | ||
|
||
@Override | ||
protected Writeable.Reader<ClusterState.Custom> instanceReader() { | ||
return ProjectStateRegistry::new; | ||
} | ||
|
||
@Override | ||
protected ClusterState.Custom createTestInstance() { | ||
return randomProjectStateRegistry(); | ||
} | ||
|
||
@Override | ||
protected ClusterState.Custom mutateInstance(ClusterState.Custom instance) throws IOException { | ||
return mutate((ProjectStateRegistry) instance); | ||
} | ||
|
||
private ProjectStateRegistry mutate(ProjectStateRegistry instance) { | ||
if (randomBoolean() && instance.size() > 0) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have separate tests for different scenarios instead of a single test that changes behavior based on randomBoolean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Theoretically yes, but this is a very common pattern in serialization tests. unless there is a certain combinations that you think is not covered? Is there something in particular that you have in mind? |
||
// Remove or mutate a project's settings or deletion flag | ||
var projectId = randomFrom(instance.getProjectsSettings().keySet()); | ||
var builder = ProjectStateRegistry.builder(instance); | ||
builder.putProjectSettings(projectId, randomSettings()); | ||
if (randomBoolean()) { | ||
// mark for deletion | ||
builder.markProjectForDeletion(projectId); | ||
} | ||
return builder.build(); | ||
} else { | ||
// add a new project | ||
return ProjectStateRegistry.builder(instance).putProjectSettings(randomUniqueProjectId(), randomSettings()).build(); | ||
} | ||
} | ||
|
||
private static ProjectStateRegistry randomProjectStateRegistry() { | ||
final var projects = randomSet(1, 5, ESTestCase::randomUniqueProjectId); | ||
final var projectsUnderDeletion = randomSet(0, 5, ESTestCase::randomUniqueProjectId); | ||
var builder = ProjectStateRegistry.builder(); | ||
projects.forEach(projectId -> builder.putProjectSettings(projectId, randomSettings())); | ||
projectsUnderDeletion.forEach( | ||
projectId -> builder.putProjectSettings(projectId, randomSettings()).markProjectForDeletion(projectId) | ||
); | ||
return builder.build(); | ||
} | ||
|
||
public static Settings randomSettings() { | ||
var builder = Settings.builder(); | ||
IntStream.range(0, randomIntBetween(1, 5)).forEach(i -> builder.put(randomIdentifier(), randomIdentifier())); | ||
return builder.build(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the "Elastic License | ||
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
* Public License v 1"; you may not use this file except in compliance with, at | ||
* your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
* License v3.0 only", or the "Server Side Public License, v 1". | ||
*/ | ||
|
||
package org.elasticsearch.cluster.project; | ||
|
||
import org.elasticsearch.test.ESTestCase; | ||
import org.hamcrest.Matchers; | ||
|
||
import static org.elasticsearch.cluster.project.ProjectStateRegistrySerializationTests.randomSettings; | ||
|
||
public class ProjectStateRegistryTests extends ESTestCase { | ||
|
||
public void testBuilder() { | ||
final var projects = randomSet(1, 5, ESTestCase::randomUniqueProjectId); | ||
final var projectsUnderDeletion = randomSet(0, 5, ESTestCase::randomUniqueProjectId); | ||
var builder = ProjectStateRegistry.builder(); | ||
projects.forEach(projectId -> builder.putProjectSettings(projectId, randomSettings())); | ||
projectsUnderDeletion.forEach( | ||
projectId -> builder.putProjectSettings(projectId, randomSettings()).markProjectForDeletion(projectId) | ||
); | ||
var projectStateRegistry = builder.build(); | ||
var gen1 = projectStateRegistry.getProjectsMarkedForDeletionGeneration(); | ||
assertThat(gen1, Matchers.equalTo(projectsUnderDeletion.isEmpty() ? 0L : 1L)); | ||
|
||
projectStateRegistry = ProjectStateRegistry.builder(projectStateRegistry).markProjectForDeletion(randomFrom(projects)).build(); | ||
var gen2 = projectStateRegistry.getProjectsMarkedForDeletionGeneration(); | ||
assertThat(gen2, Matchers.equalTo(gen1 + 1)); | ||
|
||
if (projectsUnderDeletion.isEmpty() == false) { | ||
// re-adding the same projectId should not change the generation | ||
projectStateRegistry = ProjectStateRegistry.builder(projectStateRegistry) | ||
.markProjectForDeletion(randomFrom(projectsUnderDeletion)) | ||
.build(); | ||
assertThat(projectStateRegistry.getProjectsMarkedForDeletionGeneration(), Matchers.equalTo(gen2)); | ||
} | ||
|
||
var unknownProjectId = randomUniqueProjectId(); | ||
var throwingBuilder = ProjectStateRegistry.builder(projectStateRegistry).markProjectForDeletion(unknownProjectId); | ||
assertThrows(IllegalArgumentException.class, throwingBuilder::build); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once the file-based deletion trigger is there, it should update this.