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

exec_profile: Use cluster lbp for exec profiles by default #197

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

muzarski
Copy link
Collaborator

The documentation of cass_execution_profile_set_load_balance_... functions states:

 <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.

It seems to be true, see: https://github.com/scylladb/cpp-driver/blob/master/src/request_processor.cpp#L196-L209.

We should adjust cpp-rust-driver so it behaves the same way.

This PR consist mostly of some refactor commits that prepare for the logic change in the last commit.

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.

@muzarski muzarski self-assigned this Oct 21, 2024
@muzarski muzarski mentioned this pull request Oct 21, 2024
4 tasks
scylla-rust-wrapper/src/cluster.rs Outdated Show resolved Hide resolved
scylla-rust-wrapper/src/session.rs Outdated Show resolved Hide resolved
@muzarski muzarski force-pushed the use-cluster-lbp-for-exec-profiles-by-default branch 2 times, most recently from ab0950b to a23ebac Compare October 22, 2024 09:35
@muzarski
Copy link
Collaborator Author

v2: Reverted the changes introduced by lbp_config: move LoadBalancingKind to upper layer (removed this commit). Now, the load_balancing_kind field is optional in LoadBalancingConfig.

@muzarski muzarski force-pushed the use-cluster-lbp-for-exec-profiles-by-default branch from a23ebac to 5eaa9e2 Compare October 30, 2024 12:07
@muzarski
Copy link
Collaborator Author

v3: Rebased on #200. Addressed all of @Lorak-mmk comments

`self` is consumed by this method, thus there is no need
to match dc_awareness by reference and to clone a local_dc string.
Introduced for multiple reasons:
- rack awareness will be introduced soon
- preparation for another refactor in next commit.
In this commit, we make `load_balancing_kind` optional in
`LoadBalancingConfig`. Now, there are two cases:
- for cluster's LBP, there is a default value (round-robin) which
  driver's going to use if user did not provide LBP. Thus, the `unwrap_or`
  call in `LoadBalancingConfig::build`
- for exec profile's LBP, if user did not provide an LBP kind (load_balancing_kind is None),
  then, according to cpp-driver, cluster's LBP should be used (instead of some predefined default).

After this commit, we are finally consistent with cpp-driver.
Now, if user did not specify an LBP for given execution profile,
the cluster's default LBP (session-wide LBP) will be assigned to the profile.
@muzarski muzarski force-pushed the use-cluster-lbp-for-exec-profiles-by-default branch from 5eaa9e2 to dea239a Compare October 30, 2024 18:07
@muzarski
Copy link
Collaborator Author

Rebased on master

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.

2 participants