-
Couldn't load subscription status.
- Fork 25.6k
[Persistent Tasks] Assign based on ProjectId #130391
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 12 commits
8b236cb
f8782f3
c16f55d
44e3b7c
e37d6f7
6bceb18
2b0146b
32597e4
b38a51b
7daaaef
8dfe68b
6624a94
d51ad0c
0bd72db
f66b63e
96a714f
df237f8
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 |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| package org.elasticsearch.persistent; | ||
|
|
||
| import org.elasticsearch.cluster.ClusterState; | ||
| import org.elasticsearch.cluster.metadata.ProjectId; | ||
| import org.elasticsearch.cluster.node.DiscoveryNode; | ||
| import org.elasticsearch.core.Nullable; | ||
| import org.elasticsearch.core.Tuple; | ||
|
|
@@ -63,7 +64,31 @@ public Scope scope() { | |
| * <p> | ||
| * The default implementation returns the least loaded data node from amongst the collection of candidate nodes | ||
| */ | ||
| public Assignment getAssignment(Params params, Collection<DiscoveryNode> candidateNodes, ClusterState clusterState) { | ||
| public final Assignment getAssignment( | ||
| Params params, | ||
| Collection<DiscoveryNode> candidateNodes, | ||
| ClusterState clusterState, | ||
| @Nullable ProjectId projectId | ||
| ) { | ||
| assert (scope() == Scope.PROJECT && projectId != null) || (scope() == Scope.CLUSTER && projectId == null) | ||
| : "inconsistent project-id [" + projectId + "] and task scope [" + scope() + "]"; | ||
| return doGetAssignment(params, candidateNodes, clusterState, projectId); | ||
| } | ||
|
|
||
| /** | ||
| * Returns the node id where the params has to be executed, | ||
| * <p> | ||
| * The default implementation returns the least loaded data node from amongst the collection of candidate nodes. | ||
| * <p> | ||
| * If {@link #scope()} returns CLUSTER, then {@link ProjectId} will be null. | ||
| * If {@link #scope()} returns PROJECT, then {@link ProjectId} will not be null. | ||
| */ | ||
| public Assignment doGetAssignment( | ||
|
||
| Params params, | ||
| Collection<DiscoveryNode> candidateNodes, | ||
| ClusterState clusterState, | ||
| @Nullable ProjectId projectId | ||
| ) { | ||
| DiscoveryNode discoveryNode = selectLeastLoadedNode(clusterState, candidateNodes, DiscoveryNode::canContainData); | ||
| if (discoveryNode == null) { | ||
| return NO_NODE_FOUND; | ||
|
|
@@ -105,7 +130,7 @@ protected DiscoveryNode selectLeastLoadedNode( | |
| * <p> | ||
| * Throws an exception if the supplied params cannot be executed on the cluster in the current state. | ||
| */ | ||
| public void validate(Params params, ClusterState clusterState) {} | ||
| public void validate(Params params, ClusterState clusterState, @Nullable ProjectId projectId) {} | ||
|
|
||
| /** | ||
| * Creates a AllocatedPersistentTask for communicating with task manager | ||
|
|
||
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.
We'll probably want to split this method up into a cluster-scoped version and a project-scoped version. The health node persistent task is cluster-scoped, so it doesn't really make sense to have a project ID here or in other cluster-scoped persistent tasks (even though it's nullable). Let me know if "cluster-scoped vs. project-scoped persistent tasks" sound unfamiliar to you, then I (or Yang) can explain what they are. But I'll let @ywangd decide whether he agrees or whether he's fine with the nullable project ID like this.
If we decide to split it up, we can one method without a project ID (for cluster-scoped tasks) and one with a
ProjectState(instead of aClusterStateandProjectId). We createdProjectStateto avoid passing cluster states together with project IDs.Uh oh!
There was an error while loading. Please reload this page.
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.
I'm happy to rework this, I'm not thrilled about passing nulls around. I'd be happy to instead add a method to the parent, something like:
and then
PersistentTasksClusterServicecan call theProjectStateorClusterStateAPI depending on the scope of the persistent task? I'm not sure if that is cleaner for the persistent task framework at the base level but it feels cleaner for the implementations.99% of this PR was written by IntelliJ's refactor button so we'd only be throwing away minutes of work =)
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.
Yep, something like that is what I had in mind as well (perhaps with a different name for clarity, i.e.
getProjectScopedAssignmentandgetClusterScopedAssignment, but that wouldn't be a blocker for me). Curious to hear what Yang thinks of all this.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.
Yeah we can go with two separate methods. The
PersistentTasksExecutor#scopemethod can be used to tell the scope of the executor and subsequently call the relevantgetXxxAssignmentmethod.Theoretically we can have a single overriden generic method for project and cluster scoped task executors, if
ProjectStateandClusterStateshares some interface, e.g.Supplier<ClusterState>. It should help reducing verbosity of the types. We will still need to check the task executor types and pass eitherClusterStateorProjectStateto the method accordingly. This might be something worth doing in future since it feels like a better type system. But it is definitely outside of this PR.