Skip to content

Add projectsDelta method to ClusterChangedEvent #127697

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
May 20, 2025
Merged

Add projectsDelta method to ClusterChangedEvent #127697

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2adcb2d
Add projectsDelta method to ClusterChangedEvent
ywangd May 5, 2025
fbe4b81
address feedback
ywangd May 6, 2025
e02d5d5
fix tests
ywangd May 6, 2025
bbda1f8
Merge remote-tracking branch 'origin/main' into add-projects-delta-to…
ywangd May 9, 2025
869a9e0
revert to use sets.difference
ywangd May 9, 2025
7dc1765
disable assertions since too many tests add/remove default project
ywangd May 9, 2025
77f84f6
fix test
ywangd May 9, 2025
ad8c61b
Merge remote-tracking branch 'origin/main' into add-projects-delta-to…
ywangd May 9, 2025
07576e6
Merge remote-tracking branch 'origin/main' into add-projects-delta-to…
ywangd May 10, 2025
469cdd0
Merge branch 'main' into add-projects-delta-to-cluster-changed-event
elasticmachine May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions server/src/main/java/org/elasticsearch/cluster/ClusterChangedEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,11 @@
import org.elasticsearch.cluster.metadata.IndexGraveyard.IndexGraveyardDiff;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.metadata.ProjectId;
import org.elasticsearch.cluster.metadata.ProjectMetadata;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.routing.IndexRoutingTable;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.gateway.GatewayService;
import org.elasticsearch.index.Index;

Expand All @@ -41,6 +43,8 @@ public class ClusterChangedEvent {

private final DiscoveryNodes.Delta nodesDelta;

private final ProjectsDelta projectsDelta;

public ClusterChangedEvent(String source, ClusterState state, ClusterState previousState) {
Objects.requireNonNull(source, "source must not be null");
Objects.requireNonNull(state, "state must not be null");
Expand All @@ -49,6 +53,7 @@ public ClusterChangedEvent(String source, ClusterState state, ClusterState previ
this.state = state;
this.previousState = previousState;
this.nodesDelta = state.nodes().delta(previousState.nodes());
this.projectsDelta = calculateProjectDelta(previousState.metadata(), state.metadata());
}

/**
Expand Down Expand Up @@ -237,6 +242,13 @@ public boolean nodesChanged() {
return nodesRemoved() || nodesAdded();
}

/**
* Returns the {@link ProjectsDelta} between the previous cluster state and the new cluster state.
*/
public ProjectsDelta projectDelta() {
return projectsDelta;
}

/**
* Determines whether or not the current cluster state represents an entirely
* new cluster, either when a node joins a cluster for the first time or when
Expand Down Expand Up @@ -336,4 +348,26 @@ private List<Index> indicesDeletedFromTombstones() {
.toList();
}

private static ProjectsDelta calculateProjectDelta(Metadata previousMetadata, Metadata currentMetadata) {
if (previousMetadata.projects().size() == 1
&& previousMetadata.hasProject(ProjectId.DEFAULT)
&& currentMetadata.projects().size() == 1
&& currentMetadata.hasProject(ProjectId.DEFAULT)) {
return EMPTY_PROJECT_DELTA;
}

return new ProjectsDelta(
Collections.unmodifiableSet(Sets.difference(currentMetadata.projects().keySet(), previousMetadata.projects().keySet())),
Collections.unmodifiableSet(Sets.difference(previousMetadata.projects().keySet(), currentMetadata.projects().keySet()))
);
Copy link
Contributor

@nielsbauman nielsbauman May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to construct the set differences manually, as we can construct both with one loop if we do it manually. You'll probably think this is a premature optimization, but I think the cost of the optimization is low and it's one less thing to consider optimizing later on.

Copy link
Member

@pxsalehi pxsalehi May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Also can we simplify that logic for DEFAULT? It seems looping through them and skipping default project in the loop is more readable.

Copy link
Contributor

@nielsbauman nielsbauman May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we still want that shortcut logic for single-project mode to avoid any performance impact in stateful. This wouldn't have a big impact anyway, but every bit counts IMO.

Copy link
Member Author

@ywangd ywangd May 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in fbe4b81. It should be a little more efficient since due to less call to Set#contains and less set constructions. But it essentially still has two 2 loops, 1st to initialize the removed set and 2nd to populate the added set. Please let me know whether it works for you or if you have better ideas.

we still want that shortcut logic for single-project mode

Yeah. I kept that. We don't need to skip the default project in the main loop since it should never be added or removed. I added assertions for it.

Copy link
Member Author

@ywangd ywangd May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions caused some test failures because they can create unreleasitc cluster state without the default project. Pushed e02d5d5 for fix. Let's see if there are more issues.

Copy link
Contributor

@nielsbauman nielsbauman May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah I had something else in mind but I just realized that doesn't work. You're right, we're still essentially looping twice over the data. Then I think I'd prefer your initial solution... 😅 That uses less memory as we need to copy the whole key set here. In the end it's just going to come down to memory vs. runtime, I guess. I think I'd be leaning towards saving some memory to avoid some GC work, but the differences will be minimal, so I'd be fine either way I think.

I think we can also add a shortcut for previousMetadata == currentMetadata, to skip some cluster state updates.

Copy link
Member Author

@ywangd ywangd May 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I reverted it to the original approach. Also added check for previousMetadata == currentMetadata. I had to comment out the assertion for no add/remove for default project since there are quite some tests don't do the right thing and fixing them feels a bit overwhelming for this PR.


}

private static final ProjectsDelta EMPTY_PROJECT_DELTA = new ProjectsDelta(Set.of(), Set.of());
Copy link
Member

@pxsalehi pxsalehi May 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why not move this into the record?

Copy link
Member Author

@ywangd ywangd May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in fbe4b81


public record ProjectsDelta(Set<ProjectId> added, Set<ProjectId> removed) {
public boolean isEmpty() {
return added.isEmpty() && removed.isEmpty();
}
}
}
61 changes: 61 additions & 0 deletions server/src/test/java/org/elasticsearch/cluster/ClusterChangedEventTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
import org.elasticsearch.cluster.metadata.IndexGraveyard;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.Metadata;
import org.elasticsearch.cluster.metadata.ProjectId;
import org.elasticsearch.cluster.metadata.ProjectMetadata;
import org.elasticsearch.cluster.metadata.ReservedStateMetadata;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.node.DiscoveryNodeRole;
import org.elasticsearch.cluster.node.DiscoveryNodeUtils;
Expand Down Expand Up @@ -520,6 +522,65 @@ public void testChangedCustomMetadataSetMultiProject() {
assertEquals(Set.of(IndexGraveyard.TYPE, project2Custom.getWriteableName()), event.changedCustomProjectMetadataSet());
}

public void testProjectsDelta() {
final var state0 = ClusterState.builder(TEST_CLUSTER_NAME).build();

// No project changes
final var state1 = ClusterState.builder(state0)
.metadata(Metadata.builder(state0.metadata()).put(ReservedStateMetadata.builder("test").build()))
.build();
ClusterChangedEvent event = new ClusterChangedEvent("test", state1, state0);
assertTrue(event.projectDelta().isEmpty());

// Add projects
final List<ProjectId> projectIds = randomList(1, 5, ESTestCase::randomUniqueProjectId);
Metadata.Builder metadataBuilder = Metadata.builder(state1.metadata());
for (ProjectId projectId : projectIds) {
metadataBuilder.put(ProjectMetadata.builder(projectId));
}
final var state2 = ClusterState.builder(state1).metadata(metadataBuilder.build()).build();
event = new ClusterChangedEvent("test", state2, state1);
assertThat(event.projectDelta().added(), containsInAnyOrder(projectIds.toArray()));
assertThat(event.projectDelta().removed(), empty());

// Add more projects and delete one
final var removedProjectIds = randomNonEmptySubsetOf(projectIds);
final List<ProjectId> moreProjectIds = randomList(1, 3, ESTestCase::randomUniqueProjectId);
metadataBuilder = Metadata.builder(state2.metadata());
GlobalRoutingTable.Builder routingTableBuilder = GlobalRoutingTable.builder(state2.globalRoutingTable());
for (ProjectId projectId : removedProjectIds) {
metadataBuilder.removeProject(projectId);
routingTableBuilder.removeProject(projectId);
}
for (ProjectId projectId : moreProjectIds) {
metadataBuilder.put(ProjectMetadata.builder(projectId));
}

final var state3 = ClusterState.builder(state2).metadata(metadataBuilder.build()).routingTable(routingTableBuilder.build()).build();

event = new ClusterChangedEvent("test", state3, state2);
assertThat(event.projectDelta().added(), containsInAnyOrder(moreProjectIds.toArray()));
assertThat(event.projectDelta().removed(), containsInAnyOrder(removedProjectIds.toArray()));

// Remove all projects
final List<ProjectId> remainingProjects = state3.metadata()
.projects()
.keySet()
.stream()
.filter(projectId -> ProjectId.DEFAULT.equals(projectId) == false)
.toList();
metadataBuilder = Metadata.builder(state3.metadata());
routingTableBuilder = GlobalRoutingTable.builder(state3.globalRoutingTable());
for (ProjectId projectId : remainingProjects) {
metadataBuilder.removeProject(projectId);
routingTableBuilder.removeProject(projectId);
}
final var state4 = ClusterState.builder(state3).metadata(metadataBuilder.build()).routingTable(routingTableBuilder.build()).build();
event = new ClusterChangedEvent("test", state4, state3);
assertThat(event.projectDelta().added(), empty());
assertThat(event.projectDelta().removed(), containsInAnyOrder(remainingProjects.toArray()));
}

private static class CustomClusterMetadata2 extends TestClusterCustomMetadata {
protected CustomClusterMetadata2(String data) {
super(data);
Expand Down