Skip to content
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

lbp: rack awareness #195

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

muzarski
Copy link
Collaborator

@muzarski muzarski commented Oct 17, 2024

Fix: #156
See: scylladb/cpp-driver#73

Changes

Implemented cass_[cluster/exeuction_profile]_set_load_balance_rack_aware[_n]. Adjusted the documentation, and extended unit tests for API related to LBP.

Important note about documentation and default behaviour of rack aware policy

I removed this part of the docstring:

 * With empty local_rack and local_dc,  default local_dc and local_rack
 * is chosen from the first connected contact point,
 * and no remote hosts are considered in query plans.
 * If relying on this mechanism, be sure to use only contact
 * points from the local rack.

There are multiple reasons for this:

  • this behaviour does not make much sense, and we should not mimic it IMO
  • rust-driver does not behave like this
  • this is not even true for cpp-driver

Why it's not true for cpp-driver:
If you carefully study the changes introduced to cpp-driver in the aforementioned
commit, you will notice that it's not possible for the driver to use rack aware
policy with an empty strings. This is because API functions reject
empty string, thus RackAwarePolicy object is never constructed in such case.

CassError cass_cluster_set_load_balance_rack_aware_n(CassCluster* cluster, const char* local_dc,
                                                   size_t local_dc_length,
                                                   const char* local_rack,
                                                   size_t local_rack_length) {
  if (local_dc == NULL || local_dc_length == 0 || local_rack == NULL || local_rack_length == 0) {
    return CASS_ERROR_LIB_BAD_PARAMS;
  }
  cluster->config().set_load_balancing_policy(new RackAwarePolicy(
      String(local_dc, local_dc_length), String(local_rack, local_rack_length)));
  return CASS_OK;
}

Why is this part of docstring included in cpp-driver then? No idea. Maybe,
because cass_cluster_set_load_balance_dc_aware mentions something similar
for empty (non-specified) dc. However, in this case it's true, since dc awareness is enabled
by default in cpp-driver. See the docstring:

 * Configures the cluster to use DC-aware load balancing.
 * For each query, all live nodes in a primary 'local' DC are tried first,
 * followed by any node from other DCs.
 *
 * <b>Note:</b> This is the default, and does not need to be called unless
 * switching an existing from another policy or changing settings.
 * Without further configuration, a default local_dc is chosen from the
 * first connected contact point, and no remote hosts are considered in
 * query plans. If relying on this mechanism, be sure to use only contact
 * points from the local DC.

Default node location preference

Cpp-driver, is dc-aware by default. This is not true for rust-driver, and probably will never be. In rust-driver, by default there are no node location preferences. I adjusted the documentation of cass_cluster_set_load_balance_dc_aware by removing the mention of dc awareness being enabled by default.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have implemented Rust unit tests for the features/changes introduced.
  • [ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • [ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

scylla-rust-wrapper/src/cluster.rs Outdated Show resolved Hide resolved
* first connected contact point, and no remote hosts are considered in
* query plans. If relying on this mechanism, be sure to use only contact
* points from the local DC.
*
* @deprecated The remote DC settings for DC-aware are not suitable for most
* scenarios that require DC failover. There is also unhandled gap between
* replication factor number of nodes failing and the full cluster failing. Only
Copy link
Collaborator

Choose a reason for hiding this comment

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

I generally dislike the machanism of using first contact point DC as local DC (/rack), but I wonder if this won't cause compatibility issues.

We could probably implement it by, in case dc name is empty, following mechanism:

  • After session is connected check the DC of first contact point
  • Then create new LBP with this DC set as local
  • Swap it in session using something like this:
let handle = session.get_default_execution_profile_handle().clone();
let new_profile = handle.pointee_to_builder().load_balancing_policy(...).build();
handle.map_to_another_profile(new_profile);

I'm not sure preserving this mechanism is worth introducing such trickery - probably not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow! Is it going to be the first time when profile mapping feature is actually useful (and used by anyone!) ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I said, I'm not convinced we should do this - actually, I'm more and more convinced that we shouldn't. So unfortunately it won't :(

@muzarski
Copy link
Collaborator Author

v2: Introduced NodeLocationPreferences enum. Removed DcAwareness and RackAwareness structs.

scylla-rust-wrapper/src/cluster.rs Outdated Show resolved Hide resolved
include/cassandra.h Show resolved Hide resolved
scylla-rust-wrapper/src/cluster.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/cluster.rs Outdated Show resolved Hide resolved
Comment on lines +1096 to +1097
* <b>Note:</b> Profile-based load balancing policy is disabled by default.
* cluster load balancing policy is used when profile does not contain a policy.
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can we make a profile contain a load balancing policy? This is unclear to me from this docstring; is it clear from some other documentation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cpp-rust-driver's session wrapper additionally holds a map of execution profiles, indexed by their custom names. You can firstly construct a profile, and provide custom options to it, and then pass it to CassCluster via cass_cluster_set_execution_profile with a name chosen for this profile.

Then, having multiple execution profiles defined and stored (each with unique name), you can "bind" one of your choice to some statement (see cass_statement_set_execution_profile), identifying the chosen profile by its name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I know the mechanism you described, it was me who implemented it :)
The thing is, what does it mean that a profile contains a policy?

Copy link
Collaborator Author

@muzarski muzarski Oct 21, 2024

Choose a reason for hiding this comment

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

Ok, now I understand your question. So I studied the logic of cpp-driver, and it seems that this statement is true for cpp-driver. See: https://github.com/scylladb/cpp-driver/blob/master/src/request_processor.cpp#L196-L209

If user does not define an LBP for execution profile (i.e. exec profile does not CONTAIN lbp), then the session-wide LBP is used for this execution profile. So it means, we are inconsistent with cpp-driver. Consider following scenario:

  • user sets session-wide LBP to dc aware: cass_cluster_set_load_balance_dc_aware(...)
  • user creates some exeuction profile, but does not define LBP for it (let's call this exec profile "foo")
  • now, when user executes a statement with a "foo" exec profile:
    • in cpp-driver, the session-wide DC-aware LBP is chosen for this statement execution
    • in cpp-rust-driver/rust-driver, the DefaultPolicy::default() is always chosen, no matter what session-wide LBP is set to

I believe that if we wanted to mimic this behaviour, then it's relatively easy to implement - no changes to rust-driver are required. The question is, however, whether we want to mimic such behaviour?

cc: @Lorak-mmk

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see anything wrong with such behavior. It actually seems quite useful - if you want to overwrite some things (e.g. CL and timeout) for some statements but don't want to overwrite everything. As this behavior is not buggy / footgun etc I see no reason to change it.

Actually I didn't even know that Rust Driver worked this way, I thought it behaved like cpp-driver in this regard :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened a PR: #197

Copy link
Collaborator

@wprzytula wprzytula Oct 21, 2024

Choose a reason for hiding this comment

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

Actually I didn't even know that Rust Driver worked this way, I thought it behaved like cpp-driver in this regard :D

I can vaguely remember that we decided that overriding the global per-session execution only partly with per-statement exec profile is too complex (for a user). A profile should be considered a full configuration. If only some options are to be changed for a statement compared to the session globally, it's adviced to derive an execution profile - either by cloning the builder beforehand or by calling to_builder() on an existing profile.

* first connected contact point, and no remote hosts are considered in
* query plans. If relying on this mechanism, be sure to use only contact
* points from the local DC.
*
* @deprecated The remote DC settings for DC-aware are not suitable for most
* scenarios that require DC failover. There is also unhandled gap between
* replication factor number of nodes failing and the full cluster failing. Only
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wow! Is it going to be the first time when profile mapping feature is actually useful (and used by anyone!) ?

Comment on lines 937 to 938
"eu-east\0".as_ptr() as *const i8,
"rack1\0".as_ptr() as *const i8,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use C str literals, as they've been stabilised: https://doc.rust-lang.org/nightly/edition-guide/rust-2021/c-string-literals.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done. BTW, I cannot replace let empty_str = "\0".as_ptr() as *const i8; with let empty_str = c""; It results in a following ntest::timeout proc macro panic:

error: custom attribute panicked
   --> src/cluster.rs:884:5
    |
884 |     #[ntest::timeout(100)]
    |     ^^^^^^^^^^^^^^^^^^^^^^
    |
    = help: message: Unrecognized literal: `c""`

It happens for any c str literal constructed outside of some other macro (e.g. assert_cass_error_eq).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, maybe you should open an issue to ntest?

scylla-rust-wrapper/src/cluster.rs Outdated Show resolved Hide resolved
`self` is consumed by this method, thus there is no need
to match dc_awareness by reference and to clone a local_dc string.
This is because in the following commits we will be introducing rack
awareness.
…e[_n]

This is an extension introduced by Scylla's fork of cpp-driver.
See: scylladb/cpp-driver@9691ec0

Note:
I removed this part of the docstring:
```
 * With empty local_rack and local_dc,  default local_dc and local_rack
 * is chosen from the first connected contact point,
 * and no remote hosts are considered in query plans.
 * If relying on this mechanism, be sure to use only contact
 * points from the local rack.
```

There are multiple reasons for this:
- this behaviour does not make much sense, and we should not mimic it IMO
- rust-driver does not behave like this
- this is not even true for cpp-driver

Why it's not true for cpp-driver:
If you carefully study the changes introduced to cpp-driver in the aforementioned
commit, you will notice that it's not possible for the driver to use rack aware
policy with an empty strings. This is because API functions reject
empty string, thus RackAwarePolicy object is never constructed in such case.
```
CassError cass_cluster_set_load_balance_rack_aware_n(CassCluster* cluster, const char* local_dc,
                                                   size_t local_dc_length,
                                                   const char* local_rack,
                                                   size_t local_rack_length) {
  if (local_dc == NULL || local_dc_length == 0 || local_rack == NULL || local_rack_length == 0) {
    return CASS_ERROR_LIB_BAD_PARAMS;
  }
  cluster->config().set_load_balancing_policy(new RackAwarePolicy(
      String(local_dc, local_dc_length), String(local_rack, local_rack_length)));
  return CASS_OK;
}
```

Why is this part of docstring included in cpp-driver then? No idea. Maybe,
because `cass_cluster_set_load_balance_dc_aware` mentions something similar
for empty (non-specified) dc. However, in this case it's true, since dc awareness is enabled
by default in cpp-driver. See the docstring:
```
 * Configures the cluster to use DC-aware load balancing.
 * For each query, all live nodes in a primary 'local' DC are tried first,
 * followed by any node from other DCs.
 *
 * <b>Note:</b> This is the default, and does not need to be called unless
 * switching an existing from another policy or changing settings.
 * Without further configuration, a default local_dc is chosen from the
 * first connected contact point, and no remote hosts are considered in
 * query plans. If relying on this mechanism, be sure to use only contact
 * points from the local DC.
```
…lance_rack_aware[_n]

This is an extension to the extension.
cpp-driver does not implement it for some reason.
This is not true (and I doubt it will ever be) for cpp-rust-driver.
Added test cases for rack-awareness, and extended dc-awareness
tests by empty and nullptr parameters checks.
@muzarski
Copy link
Collaborator Author

v2.1: rebased on master. (now there is only one cassadra.h header.

Still waiting for: #197

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement RackAwarePolicy loadbalancing policy
3 participants