Support consistent hashing in soft affinity node selection strategy #17115

rongrong · 2021-12-16T02:04:34Z

== RELEASE NOTES ==

General Changes
* Add support to use consistent hashing as a hash strategy when affinity scheduling is used. This feature can be turned on by setting ``node-scheduler.node-selection-hash-strategy`` to ``CONSISTENT``. A separate configuration ``node-scheduler.consistent-hashing-min-virtual-node-count`` specifies the minimal number of virtual nodes in the consistent hashing ring.

rongrong · 2021-12-16T02:05:15Z

Based on earlier work in #14738

rongrong · 2021-12-18T00:01:34Z

cc @maobaolong

beinan · 2021-12-18T01:13:52Z

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeSchedulerConfig.java

@@ -37,6 +37,8 @@
    private int maxPendingSplitsPerTask = 10;
    private int maxUnacknowledgedSplitsPerTask = 500;
    private String networkTopology = NetworkTopologyType.LEGACY;
+    private NodeSelectionHashStrategy nodeSelectionHashStrategy = NodeSelectionHashStrategy.MODULAR;


nit: should we use consistent hash as the default strategy if we didn't see any degradation in fb's staging&production?

Is there any reason for us to keep the modular strategy in the future?

We can delete modular strategy later once this is tested and rolled out fully. But to be able to safely test it we need both. And keeping the original behavior as default would make sure the feature is not accidentally on during deployment.

Agreed with Rongrong. Rolling out the new strategy will invalidate most of the existing local caches due to the shuffle of the nodes. We would like to roll it out in a more controlled manner and active monitor the clusters after it is released.

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeScheduler.java

.../src/main/java/com/facebook/presto/execution/scheduler/nodeSelection/SimpleNodeSelector.java

yuanzhanhku · 2021-12-20T19:56:20Z

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeSchedulerConfig.java

@@ -37,6 +37,8 @@
    private int maxPendingSplitsPerTask = 10;
    private int maxUnacknowledgedSplitsPerTask = 500;
    private String networkTopology = NetworkTopologyType.LEGACY;
+    private NodeSelectionHashStrategy nodeSelectionHashStrategy = NodeSelectionHashStrategy.MODULAR;


Agreed with Rongrong. Rolling out the new strategy will invalidate most of the existing local caches due to the shuffle of the nodes. We would like to roll it out in a more controlled manner and active monitor the clusters after it is released.

yuanzhanhku · 2021-12-20T19:56:20Z

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeScheduler.java

@@ -257,6 +270,13 @@ public NodeSelector createNodeSelector(Session session, ConnectorId connectorId,
                    .map(InternalNode::getNodeIdentifier)
                    .collect(toImmutableSet());

+            int weight = (int) ceil(1.0 * minVirtualNodeCount / activeNodes.size());


Where is the logic of compute the actual virtual node count based on minVirtualNodeCount and physical node count?

"weight" is basically the number of virtual nodes created for each physical node. So the actual number of virtual node is weight * physicalNodeCount = ceil(minVirtualNodeCount / physicalNodeCount) * physicalNodeCount >= minVirtualNodeCount. I figured it's better to make the number of virtual node configurable, but then the exact number is probably not important, mostly just the order of magnitudes. Directly configuring the weight is another option, but then the desired weight could be very different for a 10 node cluster vs a 500 node cluster.

Why don't we directly change the config to node-scheduler.consistent-hashing-virtual-node-count to represent the weight here?
so it would be an independent config from how many nodes a cluster have

Because, as I mentioned above, if you set weight directly, the actual number of virtual nodes on a hashing ring will depend on the number of physical node, which means it's hard to give a good default weight setting.

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeMap.java

presto-spi/src/main/java/com/facebook/presto/spi/NodeProvider.java

.../src/main/java/com/facebook/presto/execution/scheduler/nodeSelection/SimpleNodeSelector.java

kewang1024 · 2021-12-21T01:44:39Z

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeScheduler.java

+            int weight = (int) ceil(1.0 * minVirtualNodeCount / activeNodes.size());
+            for (InternalNode node : activeNodes) {
+                for (int i = 0; i < weight; i++) {
+                    activeNodesByConsistentHashing.put(murmur3_32().hashString(format("%s%d", node.getNodeIdentifier(), i), UTF_8).asInt(), node);
+                }
+            }


We should move those logic inside ConsistentHashingNodeProvider given they're specific to ConsistentHashingNodeProvider and NodeScheduler shouldn't be aware of it

In that case the map will be created for every query. Here it's created only once when node map is updated.

That's a good point, then should we actually only have one NodeProvider in the system instead of creating one for each query

I thought about that. Basically we only need to create a NodeProvider when NodeMap is changed. Currently this is cumbersome to do because when we use soft affinity with modular hashing, we are using all nodes, rather than just active nodes, to reduce reshuffling due to temporary worker unavailability. Once re remove modular hashing, and always use active nodes as candidate nodes, the NodeProvider logic can be moved into NodeMap entirely.

kewang1024

Can we add test for the ConsistentHashingNodeProvider to test the even distribution?

We have an test example in gateway, shared in here: https://docs.google.com/document/d/1C_kMut0XS8T3pBYkbAE-xsef0_206XCmMpPzbEs9Q8w/edit?usp=sharing

presto-main/src/test/java/com/facebook/presto/execution/TestNodeScheduler.java

rongrong · 2021-12-21T20:59:09Z

Can we add test for the ConsistentHashingNodeProvider to test the even distribution?

We have an test example in gateway, shared in here: https://docs.google.com/document/d/1C_kMut0XS8T3pBYkbAE-xsef0_206XCmMpPzbEs9Q8w/edit?usp=sharing

Is the test stable enough? I'm hesitant on introducing non-deterministic tests in unit test. Distribution is an attribute of the hashing function we choose. So it's not really testing our logic. Is it really necessary?

kewang1024 · 2021-12-22T01:44:24Z

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeScheduler.java

@@ -257,6 +270,13 @@ public NodeSelector createNodeSelector(Session session, ConnectorId connectorId,
                    .map(InternalNode::getNodeIdentifier)
                    .collect(toImmutableSet());

+            int weight = (int) ceil(1.0 * minVirtualNodeCount / activeNodes.size());


Why don't we directly change the config to node-scheduler.consistent-hashing-virtual-node-count to represent the weight here?
so it would be an independent config from how many nodes a cluster have

kewang1024 · 2021-12-22T01:44:24Z

.../src/main/java/com/facebook/presto/execution/scheduler/nodeSelection/SimpleNodeSelector.java

+                    if (nodeSelectionHashStrategy == MODULAR_HASHING) {
+                        nodeProvider = new ModularHashingNodeProvider(nodeMap.getAllNodes());
+                    }


I believe this part is the hacky override to make sure we're compatible with old implementation? Can we add a comment or todo here just in case people would get confused

kewang1024 · 2021-12-22T01:44:24Z

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeMap.java

+    public NodeProvider getActiveNodeProvider(NodeSelectionHashStrategy nodeSelectionHashStrategy)
+    {
+        switch (nodeSelectionHashStrategy) {
+            case MODULAR_HASHING:
+                return new ModularHashingNodeProvider(activeNodes);
+            case CONSISTENT_HASHING:
+                return new ConsistentHashingNodeProvider(activeNodesByConsistentHashing);
+            default:
+                throw new IllegalArgumentException(format("Unknown NodeSelectionHashStrategy: %s", nodeSelectionHashStrategy));
+        }
+    }


Why do we create one for each query? we can move the creation into NodeMap constructor, and getActiveNodeProvider only return the corresponding instance

In this way, we can move the specific node logic into ConsistentHashingNodeProvider (the one we were discussing in NodeScheduler.java)

Sure, but this constructor is really cheap. It doesn't really matter.

kewang1024 · 2021-12-22T01:44:24Z

presto-main/src/main/java/com/facebook/presto/execution/scheduler/NodeMap.java

+    {
+        switch (nodeSelectionHashStrategy) {
+            case MODULAR_HASHING:
+                return new ModularHashingNodeProvider(activeNodes);


Why don't we just use allNodes to keep the original logic?
then we can also remove the hacky part 166~168 in SimpleNodeSelector.java

The original logic uses activeNodes for hard affinity, and allNodes only with soft affinity. ActiveNodes should be the right one to use. allNodes is the hack.

rongrong · 2021-12-22T22:32:04Z

I added a test to test hash distribution. It's not very uniform. The node identifier and split identifier used in the test are not realistic. I don't know "random" the actual values would be and whether those would generate a better distribution.

rongrong · 2021-12-22T22:43:05Z

Had a chat with @kewang1024 offline today. I addressed the comments. There are still 2 open questions and 1 follow up:
Question 1: should we configure # virtual node in the hashing ring, or the weight of each physical node, or both.
Question 2: Choice of hashing function and distribution of hash values. I added a test to test hash distribution with fake node identifiers and fake split path. The hashing values of these fake values are not very nicely uniformly distributed. I’m not sure whether actual values would fair better on this.
Follow up: once consistent hashing is working and rolled out, we should probably delete modular hashing. There’s a hack on modular hashing to use all nodes for hashing rather than just consider active nodes, which makes the implementation of this quite awkward. If we don’t have good reasons to keep modular hashing, maybe we should just remove it later, thus removing the hack.

yuanzhanhku · 2021-12-23T00:47:14Z

...ain/src/main/java/com/facebook/presto/execution/scheduler/ConsistentHashingNodeProvider.java

+        NavigableMap<Integer, InternalNode> activeNodesByConsistentHashing = new TreeMap<>();
+        for (InternalNode node : nodes) {
+            for (int i = 0; i < weight; i++) {
+                activeNodesByConsistentHashing.put(murmur3_32().hashString(format("%s%d", node.getNodeIdentifier(), i), UTF_8).asInt(), node);


Let's move HASH_FUNCTION.hashString(format("%s%d", key, i), UTF_8).asInt() to a helper function and reuse it for hashing the node and the value, so that they are always in sync.

beinan

lgtm, thanks!

rongrong requested a review from kewang1024 December 16, 2021 02:04

rongrong force-pushed the consistent-hashing branch 2 times, most recently from 92e61ed to 708d014 Compare December 17, 2021 19:16

rongrong changed the title ~~[WIP] Support consistent hashing in soft affinity node selection strategy~~ Support consistent hashing in soft affinity node selection strategy Dec 17, 2021

rongrong force-pushed the consistent-hashing branch from 708d014 to 986a913 Compare December 17, 2021 19:23

rongrong requested review from yuanzhanhku, beinan and highker December 17, 2021 19:24

rongrong force-pushed the consistent-hashing branch from 986a913 to eebc906 Compare December 18, 2021 00:00

beinan reviewed Dec 18, 2021

View reviewed changes

yuanzhanhku reviewed Dec 20, 2021

View reviewed changes

kewang1024 reviewed Dec 21, 2021

View reviewed changes

rongrong force-pushed the consistent-hashing branch from eebc906 to 54972b5 Compare December 21, 2021 05:22

kewang1024 reviewed Dec 21, 2021

View reviewed changes

presto-main/src/test/java/com/facebook/presto/execution/TestNodeScheduler.java Show resolved Hide resolved

rongrong force-pushed the consistent-hashing branch 2 times, most recently from 39415b3 to fd11fe8 Compare December 21, 2021 20:55

rongrong force-pushed the consistent-hashing branch from fd11fe8 to 747d6f6 Compare December 21, 2021 22:40

rongrong requested review from beinan, kewang1024 and yuanzhanhku December 21, 2021 22:44

kewang1024 reviewed Dec 22, 2021

View reviewed changes

Add NodeProvider interface and ModularHashingNodeProvider

7fba104

rongrong force-pushed the consistent-hashing branch 2 times, most recently from 95705f6 to f60ea2b Compare December 22, 2021 22:29

Add ConsistentHashingNodeProvider

6bc6bf7

rongrong force-pushed the consistent-hashing branch from f60ea2b to 6bc6bf7 Compare December 22, 2021 22:37

kewang1024 approved these changes Dec 22, 2021

View reviewed changes

yuanzhanhku approved these changes Dec 23, 2021

View reviewed changes

beinan approved these changes Dec 23, 2021

View reviewed changes

beinan merged commit 123c2ab into prestodb:master Dec 23, 2021
40 checks passed

swapsmagic mentioned this pull request Dec 29, 2021

Add release notes for 0.268 #17143

Merged

1 task

beinan mentioned this pull request Apr 5, 2022

Port Consistent hash policy to alluxio Alluxio/alluxio#15262

Open

Support consistent hashing in soft affinity node selection strategy #17115

Support consistent hashing in soft affinity node selection strategy #17115

rongrong commented Dec 16, 2021 •

edited

rongrong commented Dec 16, 2021

rongrong commented Dec 18, 2021

beinan Dec 18, 2021

beinan Dec 18, 2021

rongrong Dec 20, 2021

yuanzhanhku Dec 20, 2021

yuanzhanhku Dec 20, 2021

yuanzhanhku Dec 20, 2021

rongrong Dec 21, 2021

kewang1024 Dec 22, 2021

rongrong Dec 22, 2021

kewang1024 Dec 21, 2021

rongrong Dec 21, 2021

kewang1024 Dec 21, 2021

rongrong Dec 21, 2021

kewang1024 left a comment

rongrong commented Dec 21, 2021

kewang1024 Dec 22, 2021

kewang1024 Dec 22, 2021

kewang1024 Dec 22, 2021

rongrong Dec 22, 2021

kewang1024 Dec 22, 2021

rongrong Dec 22, 2021

rongrong commented Dec 22, 2021

rongrong commented Dec 22, 2021

yuanzhanhku Dec 23, 2021

beinan left a comment

Support consistent hashing in soft affinity node selection strategy #17115

Support consistent hashing in soft affinity node selection strategy #17115

Conversation

rongrong commented Dec 16, 2021 • edited

rongrong commented Dec 16, 2021

rongrong commented Dec 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kewang1024 left a comment

Choose a reason for hiding this comment

rongrong commented Dec 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongrong commented Dec 22, 2021

rongrong commented Dec 22, 2021

Choose a reason for hiding this comment

beinan left a comment

Choose a reason for hiding this comment

rongrong commented Dec 16, 2021 •

edited