DC-aware LBP - better support the deprecated parameters #321
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,9 @@ use crate::cass_error::CassError; | |
| use crate::cass_types::CassConsistency; | ||
| use crate::exec_profile::{CassExecProfile, ExecProfileName, exec_profile_builder_modify}; | ||
| use crate::future::CassFuture; | ||
| use crate::load_balancing::{ | ||
| CassHostFilter, DcRestriction, LoadBalancingConfig, LoadBalancingKind, | ||
| }; | ||
| use crate::retry_policy::CassRetryPolicy; | ||
| use crate::ssl::CassSsl; | ||
| use crate::timestamp_generator::CassTimestampGen; | ||
|
|
@@ -826,20 +828,44 @@ pub(crate) unsafe fn set_load_balance_dc_aware_n( | |
| used_hosts_per_remote_dc: c_uint, | ||
| allow_remote_dcs_for_local_cl: cass_bool_t, | ||
| ) -> CassError { | ||
| let Some(local_dc) = (unsafe { ptr_to_cstr_n(local_dc_raw, local_dc_length) }) else { | ||
| tracing::error!( | ||
| "Provided null or non-UTF-8 local DC name to cass_*_set_load_balance_dc_aware(_n)!" | ||
| ); | ||
| return CassError::CASS_ERROR_LIB_BAD_PARAMS; | ||
| }; | ||
|
|
||
| if local_dc_length == 0 { | ||
| tracing::error!("Provided empty local DC name to cass_*_set_load_balance_dc_aware(_n)!"); | ||
| return CassError::CASS_ERROR_LIB_BAD_PARAMS; | ||
| } | ||
|
|
||
| let permit_dc_failover = if used_hosts_per_remote_dc > 0 { | ||
| // TODO: update cassandra.h documentation to reflect this behaviour. | ||
| tracing::warn!( | ||
| "cass_*_set_load_balance_dc_aware(_n): `used_hosts_per_remote_dc` parameter is only partially \ | ||
| supported in the driver: `0` is supported correctly, and any value `>0` has the semantics of \"+inf\", \ | ||
| which means no limit on the number of hosts per remote DC. This is different from the original cpp-driver! \ | ||
| To clarify, you can understand this parameter as \"permit_dc_failover\", with `0` being `false` and `>0` \ | ||
| being `true`." | ||
|
Comment on lines
+843
to
+850
There was a problem hiding this comment. I understand it is difficult to support connection-opening part of this parameter. We could expose cpp_rust_unstable method to allow that. What is the problem with exposing LBP part of that? You can wrap LBP, and make an iterator that counts returned nodes per dc, and filters out nodes of DCs that reached the limit. There was a problem hiding this comment. We could also handle the failover-for-local-cl parameter in wrapper. There was a problem hiding this comment.
It's not the matter of the Rust driver's API; it's how its connection pooling works inside.
With such flawed semantics AND a large and nontrivial effort that would be needed to do this in the Rust Driver's inner parts, I'm strongly for not supporting
Yes, we could do that. But what would be the benefit of this? I see none. The important part is not to open unnecessary connections. With connections already opened, I don't see any benefit from omitting a strict subset of remote targets from the query plan. If you see any benefit, please share it. But not seeing it, I'm not going to implement this. There was a problem hiding this comment.
Oh I'm not suggesting to follow such weird behaviors. The important part is limiting the connections to N per remote DC - if we have exact semantics more that are more reasonable than cpp-driver, even better. Ofc there is no need to do this in this PR. |
||
| ); | ||
| true | ||
| } else { | ||
| false | ||
| }; | ||
|
|
||
| let allow_remote_dcs_for_local_cl = allow_remote_dcs_for_local_cl != 0; | ||
|
|
||
| load_balancing_config.load_balancing_kind = Some(LoadBalancingKind::DcAware { | ||
| local_dc: local_dc.to_owned(), | ||
| permit_dc_failover, | ||
| allow_remote_dcs_for_local_cl, | ||
| }); | ||
| load_balancing_config.filtering.dc_restriction = if permit_dc_failover { | ||
| DcRestriction::None | ||
| } else { | ||
| DcRestriction::Local(local_dc.to_owned()) | ||
| }; | ||
|
|
||
| CassError::CASS_OK | ||
| } | ||
|
|
@@ -1757,7 +1783,7 @@ mod tests { | |
| cass_cluster_set_load_balance_dc_aware( | ||
| cluster_raw.borrow_mut(), | ||
| c"eu".as_ptr(), | ||
| 0, // forbid DC failover | ||
| 0 | ||
| ), | ||
| CassError::CASS_OK | ||
|
|
@@ -1777,8 +1803,14 @@ mod tests { | |
| let cluster = BoxFFI::as_ref(cluster_raw.borrow()).unwrap(); | ||
| let load_balancing_kind = &cluster.load_balancing_config.load_balancing_kind; | ||
| match load_balancing_kind { | ||
| Some(LoadBalancingKind::DcAware { | ||
| local_dc, | ||
| permit_dc_failover, | ||
| allow_remote_dcs_for_local_cl, | ||
| }) => { | ||
| assert_eq!(local_dc, "eu"); | ||
| assert!(!permit_dc_failover); | ||
| assert!(!allow_remote_dcs_for_local_cl); | ||
| } | ||
| _ => panic!("Expected preferred dc"), | ||
| } | ||
|
|
@@ -1814,8 +1846,8 @@ mod tests { | |
| cass_cluster_set_load_balance_dc_aware( | ||
| cluster_raw.borrow_mut(), | ||
| c"eu".as_ptr(), | ||
| 42, // allow DC failover | ||
| cass_true // allow remote DCs for local CL | ||
| ), | ||
| CassError::CASS_OK | ||
| ); | ||
|
|
@@ -1824,34 +1856,20 @@ mod tests { | |
| let node_location_preference = | ||
| &cluster.load_balancing_config.load_balancing_kind; | ||
| match node_location_preference { | ||
| Some(LoadBalancingKind::DcAware { | ||
| local_dc, | ||
| permit_dc_failover, | ||
| allow_remote_dcs_for_local_cl, | ||
| }) => { | ||
| assert_eq!(local_dc, "eu"); | ||
| assert!(permit_dc_failover); | ||
| assert!(allow_remote_dcs_for_local_cl); | ||
| } | ||
| _ => panic!("Expected preferred dc"), | ||
| } | ||
| } | ||
| /* Test invalid configurations */ | ||
| { | ||
| // null pointers | ||
| assert_cass_error_eq!( | ||
| cass_cluster_set_load_balance_dc_aware( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.