Add rack-awareness to default LBP #666

Gor027 · 2023-03-20T10:19:05Z

Added rack-awareness option to prefer replicas in local rack while picking a coordinator. It will allow to create a top-level tier with local datacenter and local rack replicas in the fallback plan.

Fixes: #604

Pre-review checklist

I have split my patch into logically separate commits.
All commit messages clearly explain what they change and why.
I added relevant tests for new features and bug fixes.
All commits compile, pass static checks and pass test.
PR description sums up the changes and reasons why they should be introduced.
I added appropriate Fixes: annotations to PR description.

scylla/src/transport/load_balancing/default.rs

piodul

You added the implementation of rack awareness, but I can't see a way to enable it from the user's perspective. The DefaultPolicyBuilder should be adjusted.

piodul · 2023-03-20T17:39:45Z

scylla/src/transport/load_balancing/default.rs

@@ -1066,6 +1104,31 @@ mod tests {
                    .group([B, C, E]) // remote nodes
                    .build(),
            },
+            // Keyspace SS with RF=2 with enabled DC failover


Please adjust this description to mention that this test also enables rack-awareness

piodul · 2023-03-20T17:42:20Z

scylla/src/transport/load_balancing/default.rs

+                //                                      us  eu  eu  us  eu  eu  us
+                //                                      r2  r1  r1  r1  r2  r1  r1
+                expected_groups: ExpectedGroupsBuilder::new()
+                    .group([G, B]) // pick + fallback local replicas


Shouldn't those be in separate groups? G is in the local rack but B is not.

Gor027 · 2023-03-21T18:53:28Z

V2:

The pick() function will prefer local rack replicas, and if there are none, it will prioritize local DC replicas.
The fallback() function will have a new tier in the load balancing order prioritizing local DC local rack replicas in the fallback plan.

Fixed all suggestions mentioned above.

piodul · 2023-03-22T10:25:56Z

Please rebase on main. The PR doesn't seem to account for the newly added option that disables replica shuffling.

wprzytula · 2023-03-22T12:49:47Z

Now that we want filter replicas not only by DC, but also by rack, we should consider adding this option to the replica locator, similarly to how filtering by DC is currently implemented. WDYT @havaker @cvybhu?
Alternatively, we can filter replicas by rack on the higher level (as this commit attempts atm), but this is an inconsistency, and a probable source of inefficiency.

havaker · 2023-03-22T12:47:40Z

scylla/src/transport/load_balancing/default.rs

+            let local_rack_replicas = self.replicas(
+                ts,
+                true,
+                |node| {
+                    Self::is_alive(node)
+                        && self.preferred_rack.is_some()
+                        && DefaultPolicy::rack_predicate(node, &self.preferred_rack)
+                },
+                cluster,
+            );
+            let local_replicas = self.shuffled_replicas(
+                ts,
+                true,
+                |node| {
+                    Self::is_alive(node)
+                        && (self.preferred_rack.is_none()
+                            || !DefaultPolicy::rack_predicate(node, &self.preferred_rack))
+                },
+                cluster,
+            );


Why are local_rack_replicas not chosen with self.shuffled_replicas?

havaker · 2023-03-22T13:02:59Z

scylla/src/transport/load_balancing/default.rs

+    fn pick_local_rack_replica<'a>(
+        &'a self,
+        ts: &TokenWithStrategy<'a>,
+        should_be_local: bool,
+        predicate: &impl Fn(&NodeRef<'a>) -> bool,
+        cluster: &'a ClusterData,
+    ) -> Option<NodeRef<'a>> {
+        if let Some(rack) = &self.preferred_rack {
+            return self
+                .nonfiltered_replica_set(ts, should_be_local, cluster)
+                .choose_filtered(&mut thread_rng(), |node| {
+                    predicate(&node) && node.rack.as_deref() == Some(rack)
+                });
+        }
+
+        None
+    }
+


In my opinion, creating a distinct function for selecting local rack replicas is unnecessary. Instead, the desired outcome can be accomplished by utilizing a suitable predicate with the pick_replicas function.

Furthermore, it appears that the modifications introduced by rebasing were not taken into account. While pick_replica has the ability to select from an unshuffled sequence, this capability is not present in pick_local_rack_replica, which results in a lack of support for the enable_shuffling_replicas(false) option.

Overall, I believe this reinforces the argument against replicating the functionality of pick_replica and instead utilizing a custom predicate to choose local rack replicas.

havaker · 2023-03-22T13:05:50Z

scylla/src/transport/load_balancing/default.rs

+                //                                      us  eu  eu  us  eu  eu  us
+                //                                      r2  r1  r1  r1  r2  r1  r1
+                expected_groups: ExpectedGroupsBuilder::new()
+                    .group([G]) // pick local rack replicas


Having a single element rack group fails to test shuffling.

Please add a test, that tests shuffling of local-rack replicas.

As I understand by reading the ExpectedGroups::assert_proper_grouping_in_plans code, it does not test the shuffling of nodes, it just verifies that the plan contains all expected nodes in the group. So, does it matter if there are one or more replicas if you collect them with group?

I think it does test shuffling.

scylla-rust-driver/scylla/src/transport/load_balancing/default.rs

Lines 707 to 715 in 758ec3e

ExpectedGroup::NonDeterministic(s) => {

// The group is supposed to have non-deterministic

// ordering. If the group size is larger than one,

// then expect there to be more than one group

// in the set.

if gots.len() > 1 && s.len() > 1 {

assert!(sets.len() > 1);

}

}

After some thought though, I realized that it would be quite difficult to test if returned local-rack replicas as shuffled. It is because local-rack replicas are always first in the plan (if available), an hence the first element from the plan is a picked local-rack replica. If pick chooses nodes in a random way but fallback doesn't, the returned group in plan is not deterministic anyway. Our testing framework has no way of detecting that a group is only partially shuffled.

I don't know if adding new functionality to the testing framework is worth it. I guess that I'm withdrawing my request for a test that checks shuffling of local-rack replicas. @wprzytula any thoughts on this?

I think it would be nice to at least verify that pick chooses rack-local replicas randomly. Having a test with the "local rack replicas" group of size 2 should check this.

Later we can consider adjusting the tests - for example, in addition to checking plans that combine pick and fallback we could also check plans returned by fallback only.

havaker · 2023-03-22T13:06:31Z

scylla/src/transport/load_balancing/default.rs

+    /// in the preferred datacenter, and then the other replicas in the datacenter.
+    ///
+    /// When a preferred datacenter is not set, setting preferred rack will not have any effect.
+    pub fn prefer_rack(mut self, rack_name: String) -> Self {


Please add this function's documentation to the doc book.

havaker · 2023-03-22T13:09:05Z

scylla/src/transport/load_balancing/default.rs

+                        && self.preferred_rack.is_some()
+                        && DefaultPolicy::rack_predicate(node, &self.preferred_rack)


I would prefer to include checking if preferred_rack is not None to rack_predicate. It would reduce the complexity of this expression.

havaker · 2023-03-22T13:18:10Z

scylla/src/transport/load_balancing/default.rs

@@ -353,6 +405,10 @@ impl DefaultPolicy {
        node.is_enabled()
    }

+    fn rack_predicate(node: &NodeRef<'_>, preferred_rack: &Option<String>) -> bool {


The other predicates are named after the condition they check, such as is_alive which verifies if a node is alive (it doesn't at the moment but will be;))). It may be worthwhile to rename rack_predicate to adhere to this convention, e.g. is_rack_local.

scylla/src/transport/load_balancing/default.rs

havaker · 2023-03-22T15:01:31Z

Now that we want filter replicas not only by DC, but also by rack, we should consider adding this option to the replica locator, similarly to how filtering by DC is currently implemented. WDYT @havaker @cvybhu? Alternatively, we can filter replicas by rack on the higher level (as this commit attempts atm), but this is an inconsistency, and a probable source of inefficiency.

I was the one to suggest filtering replicas by rack on the higher level to @Gor027;)))

The idea of putting more filtering options to the locator came to my mind, but I'm not a fan of it.
Required modifications would be complex (and API breaking):

Rack aware filtering does not fit into the current locator design as easily as DC filtering.
It is not always desired to precompute rack-filtered replica sets - session wide configuration options would need to be exposed.

To simplify the scope of this pull request I would go with the predicate-filtering. We could later assess performance gains of moving such filtering to the locator module and implement it if needed.

Gor027 · 2023-03-26T01:07:59Z

V3:

Picking a local rack replica is implemented using pick_replica as suggested above:
As @wprzytula suggested above, the should_be_local parameter in pick_replica function is replaced by ReplicaLocation enum which resulted in the following change in the function:
```
        let (predicate, should_be_local): (Box<dyn Fn(&NodeRef<'a>) -> bool>, bool) =
         match replica_location {
             ReplicaLocation::Any => (Box::new(|node| predicate(node)), false),
             ReplicaLocation::Datacenter => (Box::new(|node| predicate(node)), true),
             ReplicaLocation::DatacenterAndRack => (
                 Box::new(|node| predicate(node) && node.rack == self.preferred_rack),
                 true,
             ),
         };
```
Even though the signatures of the closures in the above match arms are the same, however, the types are different so I could not find a more elegant solution than to wrap the returned predicates in Box. As a consequence, the predicate variable has a complex type and Clippy started complaining about it, so for now, the warnings are suppressed with the allow attribute: #[allow(clippy::type_complexity)]
Added tests for rack awareness and fixed shuffling enabled
Added documentation in the docs book

scylla/src/transport/load_balancing/default.rs

havaker · 2023-03-29T11:10:06Z

scylla/src/transport/load_balancing/default.rs

@@ -13,6 +13,12 @@ use scylla_cql::{errors::QueryError, frame::types::SerialConsistency, Consistenc
 use std::{fmt, sync::Arc, time::Duration};
 use tracing::warn;

+pub enum ReplicaLocation {


Why is it public?

It was added by mistake, fixed it.

havaker · 2023-03-29T11:15:33Z

scylla/src/transport/load_balancing/default.rs

+            let local_rack_replicas = self.replicas(
+                ts,
+                true,
+                |node| {
+                    Self::is_alive(node)
+                        && self.preferred_rack.is_some()
+                        && node.rack == self.preferred_rack
+                },
+                cluster,
+            );
+            let local_replicas = self.shuffled_replicas(
+                ts,
+                true,
+                |node| {
+                    Self::is_alive(node)
+                        && (self.preferred_rack.is_none() || node.rack != self.preferred_rack)
+                },
+                cluster,
+            );


self.replicas, self.shuffled_replicas and self.pick_replica had a very similar signature. I feel that it's a bit inconsistent that you introduced a ReplicaLocation enum only in self.pick_replica. Was there any specific reason behind it?

I changed the implementation of retrieving local rack replicas to use shuffled_replicas and appropriately changed the should_be_local: bool to replica_location: ReplicaLocation enum in all of the methods.

After your changes, semantics of self.shuffled_replicas and self.pick_replica differ. self.pick_replica modifies its predicate according to the value of ReplicaLocation argument, but self.shuffled_replicas does not. Why?

You're right, the predicate logic was passed in the fallback function, so I thought it is redundant to modify the predicate in self.replicas. I fixed it, but the predicate modification required the ReplicaLocation enum to implement Clone and Copy.

scylla/src/transport/load_balancing/default.rs

havaker · 2023-03-29T11:24:27Z

scylla/src/transport/load_balancing/default.rs

+            let local_rack_replicas = self.replicas(
+                ts,
+                true,
+                |node| {
+                    Self::is_alive(node)
+                        && self.preferred_rack.is_some()
+                        && DefaultPolicy::rack_predicate(node, &self.preferred_rack)
+                },
+                cluster,
+            );
+            let local_replicas = self.shuffled_replicas(
+                ts,
+                true,
+                |node| {
+                    Self::is_alive(node)
+                        && (self.preferred_rack.is_none()
+                            || !DefaultPolicy::rack_predicate(node, &self.preferred_rack))
+                },
+                cluster,
+            );


havaker · 2023-03-29T11:25:23Z

scylla/src/transport/load_balancing/default.rs

+                //                                      us  eu  eu  us  eu  eu  us
+                //                                      r2  r1  r1  r1  r2  r1  r1
+                expected_groups: ExpectedGroupsBuilder::new()
+                    .group([G]) // pick local rack replicas


Please add a test, that tests shuffling of local-rack replicas.

havaker · 2023-03-29T11:27:00Z

docs/source/load-balancing/default-policy.md

Please update the Creating a DefaultPolicy section.

havaker · 2023-03-29T15:22:02Z

Our convention regarding commit names is something like module_name: short description of changes. I don't see you sticking to it:/

havaker · 2023-03-30T13:06:10Z

scylla/src/transport/load_balancing/default.rs

@@ -68,6 +69,21 @@ or refrain from preferring datacenters (which may ban all other datacenters, if
            }
        }
        if let Some(ts) = &routing_info.token_with_strategy {
+            // Try to pick some alive local rack random replica.
+            // If preferred rack is not specified, try to pick local DC replica.
+            if let Some(_local_rack) = &self.preferred_rack {


if let is a bit superfluous here, you are creating an unused _local_rack variable. if self.preferred_rack.is_some() would be a better choice there probably.

havaker · 2023-03-30T13:20:01Z

scylla/src/transport/load_balancing/default.rs

+        let predicate = move |node| match replica_location {
+            ReplicaLocation::Any | ReplicaLocation::Datacenter => predicate(&node),
+            ReplicaLocation::DatacenterAndRack => {
+                predicate(&node) && node.rack == self.preferred_rack
+            }
+        };


I think that it belongs to the previous commit, where you added ReplicaLocation parameter to pick_replica, replicas and shuffled_replicas.

This is because without this predicate modification, those functions are not fully respecting ReplicaLocation values - replicas(..., ReplicaLocation::DatacenterAndRack, ...) won't return a set that is restricted to local rack.

havaker · 2023-03-30T13:20:32Z

scylla/src/transport/load_balancing/default.rs

+        let predicate = |node| match replica_location {
+            ReplicaLocation::Any | ReplicaLocation::Datacenter => predicate(&node),
+            ReplicaLocation::DatacenterAndRack => {
+                predicate(&node) && node.rack == self.preferred_rack
+            }
+        };


havaker · 2023-03-30T13:22:32Z

scylla/src/transport/load_balancing/default.rs

        let should_be_local = match replica_location {
            ReplicaLocation::Any => false,
            ReplicaLocation::Datacenter | ReplicaLocation::DatacenterAndRack => true,
        };
        self.nonfiltered_replica_set(ts, should_be_local, cluster)
            .into_iter()
-            .filter(predicate)
+            .filter(move |node: &NodeRef<'a>| predicate(node))


This change should also be applied to the previous commit - it does not compile without it.

havaker · 2023-03-30T13:27:14Z

docs/source/load-balancing/default-policy.md

+prioritize nodes based on their availability zones in the preferred datacenter.
+When a preferred rack is set, the policy will first return replicas in the local rack
+in the preferred datacenter, and then the other replicas in the datacenter.
+When a preferred datacenter is not set, setting preferred rack will not have any effect.


Please add a test that checks this behavior.

Updated documentation in the docs book.

Changed `should_be_local` parameter in `pick_replica`, `replicas` and `shuffled_replicas` to `ReplicaLocation` enum. It is more expressive than a boolean and allows for more ways of filtering while picking a replica. Modified pick and fallback plan to prefer local rack replicas.

piodul · 2023-04-03T08:24:41Z

scylla/src/transport/load_balancing/default.rs

+                //                                      eu  eu  us  eu  eu  us  us
+                //                                      r1  r1  r1  r2  r1  r2  r1
+                expected_groups: ExpectedGroupsBuilder::new()
+                    .deterministic([B, C]) // pick local rack + fallback replicas


Nit: instead of this single .deterministic, you should do:

.deterministic([B]) .deterministic([C])

The .deterministic() method does not check that the load balancing policy always returns in the specified order - e.g. in your case it does not check that it always return B and then C. It only checks that the nodes within the group are always returned in an unspecified, but always the same order.

In your case, both [B, C] and [C, B] orders will be accepted by the test as long as the load balancing policy always sticks to the same order. I assume that you wanted to check that B then C is always returned - in such case you should use two groups like I suggested.

havaker

LGTM

There's one minor thing left that I think is worth fixing - the main thing transport: load_balancing: Add ReplicationLocation enum commit introduces is rack-awareness to the default policy internals, but it's not mentioned anywhere in the commit message.

Gor027 · 2023-04-05T10:02:22Z

LGTM

There's one minor thing left that I think is worth fixing - the main thing transport: load_balancing: Add ReplicationLocation enum commit introduces is rack-awareness to the default policy internals, but it's not mentioned anywhere in the commit message.

It's mentioned in a short sentence: Modified pick and fallback plan to prefer local rack replicas.

Gor027 force-pushed the rack-awareness branch from 6d307cd to df802a3 Compare March 20, 2023 10:19

Gor027 requested review from havaker, wprzytula and piodul March 20, 2023 16:30

wprzytula requested changes Mar 20, 2023

View reviewed changes

scylla/src/transport/load_balancing/default.rs Outdated Show resolved Hide resolved

scylla/src/transport/load_balancing/default.rs Outdated Show resolved Hide resolved

piodul reviewed Mar 20, 2023

View reviewed changes

Gor027 force-pushed the rack-awareness branch from df802a3 to 1373b17 Compare March 21, 2023 18:45

Gor027 force-pushed the rack-awareness branch 2 times, most recently from 1e9e7cf to acb2eda Compare March 22, 2023 10:36

havaker suggested changes Mar 22, 2023

View reviewed changes

wprzytula requested changes Mar 22, 2023

View reviewed changes

scylla/src/transport/load_balancing/default.rs Outdated Show resolved Hide resolved

scylla/src/transport/load_balancing/default.rs Outdated Show resolved Hide resolved

scylla/src/transport/load_balancing/default.rs Outdated Show resolved Hide resolved

Gor027 force-pushed the rack-awareness branch from acb2eda to 5467a09 Compare March 26, 2023 01:07

wprzytula reviewed Mar 26, 2023

View reviewed changes

scylla/src/transport/load_balancing/default.rs Outdated Show resolved Hide resolved

Gor027 force-pushed the rack-awareness branch from 5467a09 to 42aec60 Compare March 26, 2023 21:53

Gor027 requested review from havaker, piodul and wprzytula March 28, 2023 14:07

Gor027 force-pushed the rack-awareness branch from 42aec60 to db74262 Compare March 28, 2023 14:17

havaker suggested changes Mar 29, 2023

View reviewed changes

Gor027 force-pushed the rack-awareness branch from db74262 to a203a62 Compare March 29, 2023 12:43

Gor027 force-pushed the rack-awareness branch 3 times, most recently from 280a1e1 to 90edd07 Compare March 30, 2023 12:34

havaker suggested changes Mar 30, 2023

View reviewed changes

Gor027 force-pushed the rack-awareness branch from 90edd07 to 15f05f8 Compare March 30, 2023 16:12

Gor Stepanyan added 2 commits April 2, 2023 21:17

transport: load_balancing: Add preferred_rack field to Default LBP

8a54dbc

Updated documentation in the docs book.

Gor027 force-pushed the rack-awareness branch from 15f05f8 to 2757d67 Compare April 2, 2023 19:17

piodul reviewed Apr 3, 2023

View reviewed changes

transport: load_balancing: Add tests for rack-awareness

6aa105d

Gor027 force-pushed the rack-awareness branch from 2757d67 to 6aa105d Compare April 3, 2023 11:12

Gor027 requested review from piodul and havaker April 4, 2023 08:02

havaker approved these changes Apr 5, 2023

View reviewed changes

piodul approved these changes Apr 5, 2023

View reviewed changes

piodul merged commit 91a1b8e into scylladb:main Apr 5, 2023
8 checks passed

	ExpectedGroup::NonDeterministic(s) => {
	// The group is supposed to have non-deterministic
	// ordering. If the group size is larger than one,
	// then expect there to be more than one group
	// in the set.
	if gots.len() > 1 && s.len() > 1 {
	assert!(sets.len() > 1);
	}
	}

		&& self.preferred_rack.is_some()
		&& DefaultPolicy::rack_predicate(node, &self.preferred_rack)

Add rack-awareness to default LBP #666

Add rack-awareness to default LBP #666

Conversation

Gor027 commented Mar 20, 2023

Pre-review checklist

piodul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gor027 commented Mar 21, 2023

piodul commented Mar 22, 2023

wprzytula commented Mar 22, 2023

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

havaker commented Mar 22, 2023 • edited

Gor027 commented Mar 26, 2023

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

havaker commented Mar 29, 2023

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

havaker left a comment

Choose a reason for hiding this comment

Gor027 commented Apr 5, 2023

havaker commented Mar 22, 2023 •

edited