CASSCPP-5 cass_cluster_set_load_balance_dc_aware_n fails with NULL/empty local DC patch by Bret McGuire; reviewed by Bret McGuire and Yifan Cai
diff --git a/include/cassandra.h b/include/cassandra.h index 6d1ac7a..c51bfa2 100644 --- a/include/cassandra.h +++ b/include/cassandra.h
@@ -2141,6 +2141,10 @@ * For each query, all live nodes in a primary 'local' DC are tried first, * followed by any node from other DCs. * + * <b>Important:</b> Pass in NULL for local_dc to automatically use the data center of the first + * connected control connection host as the primary data center. This approach is recommended + * over manually selecting the local_dc. + * * <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 @@ -2156,7 +2160,7 @@ * @public @memberof CassCluster * * @param[in] cluster - * @param[in] local_dc The primary data center to try first + * @param[in] local_dc The primary data center to try first, or NULL to use the DC of the first connected node * @param[in] used_hosts_per_remote_dc The number of hosts used in each remote * DC if no hosts are available in the local dc (<b>deprecated</b>) * @param[in] allow_remote_dcs_for_local_cl Allows remote hosts to be used if no @@ -2175,6 +2179,10 @@ * Same as cass_cluster_set_load_balance_dc_aware(), but with lengths for string * parameters. * + * <b>Important:</b> If local_dc is NULL or local_dc_length is 0, the data center of + * the first connected control connection host will be automatically used as the + * primary data center. This approach is recommended over manually selecting 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 @@ -2183,11 +2191,11 @@ * @public @memberof CassCluster * * @param[in] cluster - * @param[in] local_dc - * @param[in] local_dc_length + * @param[in] local_dc Primary data center to try first, or NULL to use the DC of the first connected node + * @param[in] local_dc_length Length of local_dc string * @param[in] used_hosts_per_remote_dc (<b>deprecated</b>) * @param[in] allow_remote_dcs_for_local_cl (<b>deprecated</b>) - * @return same as cass_cluster_set_load_balance_dc_aware() + * @return CASS_OK if successful, otherwise an error occurred * * @see cass_cluster_set_load_balance_dc_aware() */
diff --git a/src/cluster_config.cpp b/src/cluster_config.cpp index ddeae12..5dcc130 100644 --- a/src/cluster_config.cpp +++ b/src/cluster_config.cpp
@@ -283,9 +283,6 @@ CassError cass_cluster_set_load_balance_dc_aware(CassCluster* cluster, const char* local_dc, unsigned used_hosts_per_remote_dc, cass_bool_t allow_remote_dcs_for_local_cl) { - if (local_dc == NULL) { - return CASS_ERROR_LIB_BAD_PARAMS; - } return cass_cluster_set_load_balance_dc_aware_n(cluster, local_dc, SAFE_STRLEN(local_dc), used_hosts_per_remote_dc, allow_remote_dcs_for_local_cl); @@ -295,11 +292,13 @@ size_t local_dc_length, unsigned used_hosts_per_remote_dc, cass_bool_t allow_remote_dcs_for_local_cl) { - if (local_dc == NULL || local_dc_length == 0) { - return CASS_ERROR_LIB_BAD_PARAMS; - } + // If local_dc is NULL or length is 0, we use an empty string which causes the driver + // to use the DC of the first connected node + String dc_name = (local_dc != NULL && local_dc_length > 0) ? + String(local_dc, local_dc_length) : String(); + cluster->config().set_load_balancing_policy(new DCAwarePolicy( - String(local_dc, local_dc_length), used_hosts_per_remote_dc, !allow_remote_dcs_for_local_cl)); + dc_name, used_hosts_per_remote_dc, !allow_remote_dcs_for_local_cl)); return CASS_OK; }
diff --git a/tests/src/integration/tests/test_cluster.cpp b/tests/src/integration/tests/test_cluster.cpp index a21ca6d..41dbb60 100644 --- a/tests/src/integration/tests/test_cluster.cpp +++ b/tests/src/integration/tests/test_cluster.cpp
@@ -24,16 +24,26 @@ }; /** - * Set local dc to null for dc-aware lbp + * Set local dc to null or empty for dc-aware lbp * * @jira_ticket CPP-368 * @test_category configuration - * @expected_result Error out because it is illegal to specify a null local-dc. + * @expected_result Success because NULL or empty local-dc now means use the DC of the first connected node. */ -CASSANDRA_INTEGRATION_TEST_F(ClusterTests, SetLoadBalanceDcAwareNullLocalDc) { - test::driver::Cluster cluster; - EXPECT_EQ(CASS_ERROR_LIB_BAD_PARAMS, - cass_cluster_set_load_balance_dc_aware(cluster.get(), NULL, 99, cass_false)); +CASSANDRA_INTEGRATION_TEST_F(ClusterTests, SetLoadBalanceDcAwareNullOrEmptyLocalDc) { + // Test with NULL local_dc + { + test::driver::Cluster cluster; + EXPECT_EQ(CASS_OK, + cass_cluster_set_load_balance_dc_aware(cluster.get(), NULL, 99, cass_false)); + } + + // Test with empty string local_dc + { + test::driver::Cluster cluster; + EXPECT_EQ(CASS_OK, + cass_cluster_set_load_balance_dc_aware(cluster.get(), "", 99, cass_false)); + } } /**
diff --git a/tests/src/unit/tests/test_cluster_config.cpp b/tests/src/unit/tests/test_cluster_config.cpp new file mode 100644 index 0000000..73eaa00 --- /dev/null +++ b/tests/src/unit/tests/test_cluster_config.cpp
@@ -0,0 +1,160 @@ +/* + Copyright (c) DataStax, Inc. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +#include <gtest/gtest.h> + +#include "cluster_config.hpp" +#include "cassandra.h" +#include "dc_aware_policy.hpp" +#include "string.hpp" + +using namespace datastax; +using namespace datastax::internal; +using namespace datastax::internal::core; + +class ClusterConfigUnitTest : public testing::Test { +public: + ClusterConfigUnitTest() {} + + virtual void SetUp() { + cluster_ = cass_cluster_new(); + } + + virtual void TearDown() { + cass_cluster_free(cluster_); + } + +protected: + CassCluster* cluster_; + + const DCAwarePolicy* build_dc_policy() { + + // Disable token-aware routing in this case in order to avoid illegal instructions + // when creating a TokenAwarePolicy in build_load_balancing_policy() + cluster_->config().set_token_aware_routing(false); + cluster_->config().default_profile().build_load_balancing_policy(); + + // Verify the policy was set correctly with the right datacenter name + const LoadBalancingPolicy* policy = + cluster_->config().load_balancing_policy().get(); + return static_cast<const DCAwarePolicy*>(policy); + } +}; + +// ==================== cass_cluster_set_load_balance_dc_aware_n ==================== +TEST_F(ClusterConfigUnitTest, SetLoadBalanceDcAwareNHappyPath) { + + // Test valid parameters + const char* valid_dc = "my_datacenter"; + EXPECT_EQ(CASS_OK, cass_cluster_set_load_balance_dc_aware_n( + cluster_, valid_dc, strlen(valid_dc), + 2, cass_true)); + + const DCAwarePolicy* dc_policy = build_dc_policy(); + + // Should be using the partial string as local DC + EXPECT_EQ(dc_policy->local_dc(), valid_dc); + EXPECT_EQ(dc_policy->used_hosts_per_remote_dc(), 2u); + EXPECT_FALSE(dc_policy->skip_remote_dcs_for_local_cl()); +} + +TEST_F(ClusterConfigUnitTest, SetLoadBalanceDcAwareNWithNullLocalDc) { + + // Test with NULL pointer (should now succeed and use empty string for DC) + EXPECT_EQ(CASS_OK, + cass_cluster_set_load_balance_dc_aware_n( + cluster_, NULL, 10, 2, cass_true)); + + const DCAwarePolicy* dc_policy = build_dc_policy(); + + EXPECT_EQ(dc_policy->local_dc(), String()); + EXPECT_EQ(dc_policy->used_hosts_per_remote_dc(), 2u); + EXPECT_FALSE(dc_policy->skip_remote_dcs_for_local_cl()); +} + +TEST_F(ClusterConfigUnitTest, SetLoadBalanceDcAwareNWithZeroLengthLocalDc) { + + // Test with zero length (should now succeed and use empty string for DC) + const char* valid_dc = "my_datacenter"; + EXPECT_EQ(CASS_OK, + cass_cluster_set_load_balance_dc_aware_n( + cluster_, valid_dc, 0, 2, cass_true)); + + const DCAwarePolicy* dc_policy = build_dc_policy(); + + EXPECT_EQ(dc_policy->local_dc(), String()); + EXPECT_EQ(dc_policy->used_hosts_per_remote_dc(), 2u); + EXPECT_FALSE(dc_policy->skip_remote_dcs_for_local_cl()); +} + +TEST_F(ClusterConfigUnitTest, SetLoadBalanceDcAwareNWithEmptyLocalDc) { + + // Test with empty string (should now succeed) + const char* empty_string = ""; + EXPECT_EQ(CASS_OK, + cass_cluster_set_load_balance_dc_aware_n( + cluster_, empty_string, strlen(empty_string), 2, cass_true)); + + const DCAwarePolicy* dc_policy = build_dc_policy(); + + EXPECT_EQ(dc_policy->local_dc(), String()); + EXPECT_EQ(dc_policy->used_hosts_per_remote_dc(), 2u); + EXPECT_FALSE(dc_policy->skip_remote_dcs_for_local_cl()); +} + +TEST_F(ClusterConfigUnitTest, SetLoadBalanceDcAwareNWithPartialStringLocalDc) { + + // Test with partial string length + const char* long_dc_name = "my_datacenter_with_a_long_name"; + size_t partial_length = 5; + EXPECT_EQ(CASS_OK, cass_cluster_set_load_balance_dc_aware_n( + cluster_, long_dc_name, partial_length, + 2, cass_true)); + + const DCAwarePolicy* dc_policy = build_dc_policy(); + + // Should just use up to partial_length of the datacenter name here. + EXPECT_EQ(dc_policy->local_dc(), String(long_dc_name, partial_length)); + EXPECT_EQ(dc_policy->used_hosts_per_remote_dc(), 2u); + EXPECT_FALSE(dc_policy->skip_remote_dcs_for_local_cl()); +} + +// ==================== cass_cluster_set_load_balance_dc_aware ==================== +TEST_F(ClusterConfigUnitTest, SetLoadBalanceDcAwareWithNullLocalDc) { + // Test with NULL to use local DC from connected node + EXPECT_EQ(CASS_OK, cass_cluster_set_load_balance_dc_aware( + cluster_, NULL, 3, cass_false)); + + const DCAwarePolicy* dc_policy = build_dc_policy(); + + // Should be using empty string as local DC (will be determined at runtime) + EXPECT_EQ(dc_policy->local_dc(), String()); + EXPECT_EQ(dc_policy->used_hosts_per_remote_dc(), 3u); + EXPECT_TRUE(dc_policy->skip_remote_dcs_for_local_cl()); +} + +TEST_F(ClusterConfigUnitTest, SetLoadBalanceDcAwareWithEmptyLocalDc) { + // Test with empty string to use local DC from connected node + EXPECT_EQ(CASS_OK, cass_cluster_set_load_balance_dc_aware( + cluster_, "", 2, cass_true)); + + const DCAwarePolicy* dc_policy = build_dc_policy(); + + // Should be using empty string as local DC (will be determined at runtime) + EXPECT_EQ(dc_policy->local_dc(), String()); + EXPECT_EQ(dc_policy->used_hosts_per_remote_dc(), 2u); + EXPECT_FALSE(dc_policy->skip_remote_dcs_for_local_cl()); +}