-
Notifications
You must be signed in to change notification settings - Fork 5.5k
Make number of preferred nodes configurable #22562
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
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 |
|---|---|---|
|
|
@@ -24,8 +24,6 @@ | |
| import java.util.Optional; | ||
| import java.util.Set; | ||
|
|
||
| import static java.lang.String.format; | ||
|
|
||
| public class NodeMap | ||
| { | ||
| private final Map<String, InternalNode> activeNodesByNodeId; | ||
|
|
@@ -92,15 +90,12 @@ public SetMultimap<HostAddress, InternalNode> getAllNodesByHostAndPort() | |
| return allNodesByHostAndPort; | ||
| } | ||
|
|
||
| public NodeProvider getActiveNodeProvider(NodeSelectionHashStrategy nodeSelectionHashStrategy) | ||
| public NodeProvider getNodeProvider(int nodeCount) | ||
| { | ||
| switch (nodeSelectionHashStrategy) { | ||
| case MODULAR_HASHING: | ||
| return new ModularHashingNodeProvider(activeNodes); | ||
| case CONSISTENT_HASHING: | ||
| return consistentHashingNodeProvider.get(); | ||
| default: | ||
| throw new IllegalArgumentException(format("Unknown NodeSelectionHashStrategy: %s", nodeSelectionHashStrategy)); | ||
| if (consistentHashingNodeProvider.isPresent()) { | ||
| return (key) -> consistentHashingNodeProvider.get().get(key, nodeCount); | ||
|
Contributor
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 feels little hacky ..to plugin the node count dynamically. |
||
| } | ||
| ModularHashingNodeProvider modularHashingNodeProvider = new ModularHashingNodeProvider(allNodes); | ||
| return (key) -> modularHashingNodeProvider.get(key, nodeCount); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -18,10 +18,8 @@ | |||||||||
| import com.facebook.presto.execution.RemoteTask; | ||||||||||
| import com.facebook.presto.execution.scheduler.BucketNodeMap; | ||||||||||
| import com.facebook.presto.execution.scheduler.InternalNodeInfo; | ||||||||||
| import com.facebook.presto.execution.scheduler.ModularHashingNodeProvider; | ||||||||||
| import com.facebook.presto.execution.scheduler.NodeAssignmentStats; | ||||||||||
| import com.facebook.presto.execution.scheduler.NodeMap; | ||||||||||
| import com.facebook.presto.execution.scheduler.NodeSelectionHashStrategy; | ||||||||||
| import com.facebook.presto.execution.scheduler.SplitPlacementResult; | ||||||||||
| import com.facebook.presto.metadata.InternalNode; | ||||||||||
| import com.facebook.presto.metadata.InternalNodeManager; | ||||||||||
|
|
@@ -53,7 +51,6 @@ | |||||||||
| import static com.facebook.presto.execution.scheduler.NodeScheduler.selectExactNodes; | ||||||||||
| import static com.facebook.presto.execution.scheduler.NodeScheduler.selectNodes; | ||||||||||
| import static com.facebook.presto.execution.scheduler.NodeScheduler.toWhenHasSplitQueueSpaceFuture; | ||||||||||
| import static com.facebook.presto.execution.scheduler.NodeSelectionHashStrategy.MODULAR_HASHING; | ||||||||||
| import static com.facebook.presto.metadata.InternalNode.NodeStatus.DEAD; | ||||||||||
| import static com.facebook.presto.spi.StandardErrorCode.NODE_SELECTION_NOT_SUPPORTED; | ||||||||||
| import static com.facebook.presto.spi.StandardErrorCode.NO_NODES_AVAILABLE; | ||||||||||
|
|
@@ -80,7 +77,7 @@ public class SimpleNodeSelector | |||||||||
| private final long maxPendingSplitsWeightPerTask; | ||||||||||
| private final int maxUnacknowledgedSplitsPerTask; | ||||||||||
| private final int maxTasksPerStage; | ||||||||||
| private final NodeSelectionHashStrategy nodeSelectionHashStrategy; | ||||||||||
| private final int maxPreferredNodes; | ||||||||||
|
|
||||||||||
| public SimpleNodeSelector( | ||||||||||
| InternalNodeManager nodeManager, | ||||||||||
|
|
@@ -93,7 +90,7 @@ public SimpleNodeSelector( | |||||||||
| long maxPendingSplitsWeightPerTask, | ||||||||||
| int maxUnacknowledgedSplitsPerTask, | ||||||||||
| int maxTasksPerStage, | ||||||||||
| NodeSelectionHashStrategy nodeSelectionHashStrategy) | ||||||||||
| int maxPreferredNodes) | ||||||||||
| { | ||||||||||
| this.nodeManager = requireNonNull(nodeManager, "nodeManager is null"); | ||||||||||
| this.nodeSelectionStats = requireNonNull(nodeSelectionStats, "nodeSelectionStats is null"); | ||||||||||
|
|
@@ -106,7 +103,7 @@ public SimpleNodeSelector( | |||||||||
| this.maxUnacknowledgedSplitsPerTask = maxUnacknowledgedSplitsPerTask; | ||||||||||
| checkArgument(maxUnacknowledgedSplitsPerTask > 0, "maxUnacknowledgedSplitsPerTask must be > 0, found: %s", maxUnacknowledgedSplitsPerTask); | ||||||||||
| this.maxTasksPerStage = maxTasksPerStage; | ||||||||||
| this.nodeSelectionHashStrategy = requireNonNull(nodeSelectionHashStrategy, "nodeSelectionHashStrategy is null"); | ||||||||||
| this.maxPreferredNodes = maxPreferredNodes; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| @Override | ||||||||||
|
|
@@ -152,8 +149,7 @@ public SplitPlacementResult computeAssignments(Set<Split> splits, List<RemoteTas | |||||||||
| Set<InternalNode> blockedExactNodes = new HashSet<>(); | ||||||||||
| boolean splitWaitingForAnyNode = false; | ||||||||||
|
|
||||||||||
| NodeProvider nodeProvider = nodeMap.getActiveNodeProvider(nodeSelectionHashStrategy); | ||||||||||
|
|
||||||||||
| NodeProvider nodeProvider = nodeMap.getNodeProvider(maxPreferredNodes); | ||||||||||
| OptionalInt preferredNodeCount = OptionalInt.empty(); | ||||||||||
| for (Split split : splits) { | ||||||||||
| List<InternalNode> candidateNodes; | ||||||||||
|
|
@@ -163,10 +159,6 @@ public SplitPlacementResult computeAssignments(Set<Split> splits, List<RemoteTas | |||||||||
| preferredNodeCount = OptionalInt.of(candidateNodes.size()); | ||||||||||
| break; | ||||||||||
| case SOFT_AFFINITY: | ||||||||||
| // Using all nodes for soft affinity scheduling with modular hashing because otherwise temporarily down nodes would trigger too much rehashing | ||||||||||
| if (nodeSelectionHashStrategy == MODULAR_HASHING) { | ||||||||||
| nodeProvider = new ModularHashingNodeProvider(nodeMap.getAllNodes()); | ||||||||||
| } | ||||||||||
|
Comment on lines
-166
to
-169
Member
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. Is it intentional to use
Member
Author
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. Great question.
For The only place when a potential change is possible is when When
However I agree, the interface today makes it rather obscure. I guess a better interface could've been something like
Member
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. Thank you for the detailed message. Now I get that in practice, the situation is indeed as you said. And I agree with you that a follow-up refactor to make the interface more explicit would be great. LGTM, thanks! |
||||||||||
| candidateNodes = selectExactNodes(nodeMap, split.getPreferredNodes(nodeProvider), includeCoordinator); | ||||||||||
| preferredNodeCount = OptionalInt.of(candidateNodes.size()); | ||||||||||
| candidateNodes = ImmutableList.<InternalNode>builder() | ||||||||||
|
|
||||||||||
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.
why are we removing this ? so that we can plugin the nodeCount preference dynamically .. seems hacky
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 actually not sure if
NodeProviderinterface is necessary. This interface seems a little counterintuitive to me. ANodeProvideis passed to the split just to be called with a key. I wonder if a better interface would be to have something likeConnectorSplit#getSoftAffinityNodeSelectionKeythat returns a String (or an object) similar to what is done for fragment level caching withConnectorSplit#getSplitIdentifier. Then a scheduler can make a decision on it's own of how many preferred nodes are required. Thoughts?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.
Another thing what this PR changes fundamentally, it moves the number of preferred nodes selection from a connector to scheduler. Previously it was a connector responsibility to request a certain number of preferred nodes. Now the scheduler will decide.
From a practicality standpoint it feels like it's easier to make this selection on coordinator (a single property for every connector). But I wonder if there was some deeper thought process behind this that motivated to implement it this way.
@NikhilCollooru can you think of a situation when we think it would actually be preferred for a connector to chose of how many soft affinity nodes to chose for a certain split?
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 i dont see a usecase where you configure the node count based on connector.
so moving the logic to scheduler seems the better way to do it.
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.
Sounds good. Let me follow up with a refactor PR.