From 9d03117fbcf4c2a35604b28c4e9e96ed15414582 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 22 May 2023 08:19:44 -0700 Subject: [PATCH] [ring hash] revert 64-to-32-bit changes for objc (#33201) This reverts the changes made to ring_hash in #29872. The comment in the picker code specifically says not to change these variables to an unsigned type, but that's exactly what that PR did. I don't know if this has actually been causing any problems, but given that we duplicated this algorithm (and that comment) from elsewhere without doing a detailed analysis of it, it seems prudent to stick with the types that the original code suggested were important. To avoid causing problems for ObjC, I have changed this such that ring_hash is not built unless we are building with xDS support, which we exclude on mobile. Currently, there is no way to use ring_hash without xDS, although that might change in the future; if it does, we can deal with any problems that arise at that point. --- BUILD | 2 +- CMakeLists.txt | 1 - Makefile | 2 +- build_autogenerated.yaml | 3 --- grpc.gyp | 1 - .../client_channel/lb_policy/ring_hash/ring_hash.cc | 10 +++++----- src/core/plugin_registry/grpc_plugin_registry.cc | 2 -- src/core/plugin_registry/grpc_plugin_registry_extra.cc | 2 ++ 8 files changed, 9 insertions(+), 14 deletions(-) diff --git a/BUILD b/BUILD index 07eaddaffd9..42dbc453ad0 100644 --- a/BUILD +++ b/BUILD @@ -585,6 +585,7 @@ GRPC_XDS_TARGETS = [ "//src/core:grpc_lb_policy_xds_cluster_resolver", "//src/core:grpc_lb_policy_xds_override_host", "//src/core:grpc_lb_policy_xds_wrr_locality", + "//src/core:grpc_lb_policy_ring_hash", "//src/core:grpc_resolver_xds", "//src/core:grpc_resolver_c2p", "//src/core:grpc_xds_server_config_fetcher", @@ -807,7 +808,6 @@ grpc_cc_library( "//src/core:grpc_lb_policy_outlier_detection", "//src/core:grpc_lb_policy_pick_first", "//src/core:grpc_lb_policy_priority", - "//src/core:grpc_lb_policy_ring_hash", "//src/core:grpc_lb_policy_round_robin", "//src/core:grpc_lb_policy_weighted_round_robin", "//src/core:grpc_lb_policy_weighted_target", diff --git a/CMakeLists.txt b/CMakeLists.txt index aa7602fdfca..593e69c6a92 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2645,7 +2645,6 @@ add_library(grpc_unsecure src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc src/core/ext/filters/client_channel/lb_policy/priority/priority.cc - src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc src/core/ext/filters/client_channel/lb_policy/rls/rls.cc src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.cc diff --git a/Makefile b/Makefile index f3fd553ebb2..c950d86e080 100644 --- a/Makefile +++ b/Makefile @@ -1879,7 +1879,6 @@ LIBGRPC_UNSECURE_SRC = \ src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc \ src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc \ src/core/ext/filters/client_channel/lb_policy/priority/priority.cc \ - src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc \ src/core/ext/filters/client_channel/lb_policy/rls/rls.cc \ src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc \ src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.cc \ @@ -3039,6 +3038,7 @@ ifneq ($(OPENSSL_DEP),) # This is to ensure the embedded OpenSSL is built beforehand, properly # installing headers to their final destination on the drive. We need this # otherwise parallel compilation will fail if a source is compiled first. +src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc: $(OPENSSL_DEP) src/core/ext/filters/client_channel/lb_policy/xds/cds.cc: $(OPENSSL_DEP) src/core/ext/filters/client_channel/lb_policy/xds/xds_attributes.cc: $(OPENSSL_DEP) src/core/ext/filters/client_channel/lb_policy/xds/xds_cluster_impl.cc: $(OPENSSL_DEP) diff --git a/build_autogenerated.yaml b/build_autogenerated.yaml index bde37db8ba8..12fa8f8905d 100644 --- a/build_autogenerated.yaml +++ b/build_autogenerated.yaml @@ -1953,7 +1953,6 @@ libs: - src/core/ext/filters/client_channel/lb_policy/oob_backend_metric.h - src/core/ext/filters/client_channel/lb_policy/oob_backend_metric_internal.h - src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.h - - src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.h - src/core/ext/filters/client_channel/lb_policy/subchannel_list.h - src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.h - src/core/ext/filters/client_channel/local_subchannel_pool.h @@ -2332,7 +2331,6 @@ libs: - src/core/tsi/transport_security.h - src/core/tsi/transport_security_grpc.h - src/core/tsi/transport_security_interface.h - - third_party/xxhash/xxhash.h src: - src/core/ext/filters/backend_metrics/backend_metric_filter.cc - src/core/ext/filters/census/grpc_context.cc @@ -2362,7 +2360,6 @@ libs: - src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc - src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc - src/core/ext/filters/client_channel/lb_policy/priority/priority.cc - - src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc - src/core/ext/filters/client_channel/lb_policy/rls/rls.cc - src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc - src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.cc diff --git a/grpc.gyp b/grpc.gyp index a5a94315ddf..e9722e6f3f2 100644 --- a/grpc.gyp +++ b/grpc.gyp @@ -1121,7 +1121,6 @@ 'src/core/ext/filters/client_channel/lb_policy/outlier_detection/outlier_detection.cc', 'src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc', 'src/core/ext/filters/client_channel/lb_policy/priority/priority.cc', - 'src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc', 'src/core/ext/filters/client_channel/lb_policy/rls/rls.cc', 'src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc', 'src/core/ext/filters/client_channel/lb_policy/weighted_round_robin/static_stride_scheduler.cc', diff --git a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc index 78c78609262..d104dbe520e 100644 --- a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc +++ b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc @@ -360,12 +360,12 @@ RingHash::PickResult RingHash::Picker::Pick(PickArgs args) { // Ported from https://github.com/RJ/ketama/blob/master/libketama/ketama.c // (ketama_get_server) NOTE: The algorithm depends on using signed integers // for lowp, highp, and first_index. Do not change them! - size_t lowp = 0; - size_t highp = ring.size(); - size_t first_index = 0; + int64_t lowp = 0; + int64_t highp = ring.size(); + int64_t first_index = 0; while (true) { first_index = (lowp + highp) / 2; - if (first_index == ring.size()) { + if (first_index == static_cast(ring.size())) { first_index = 0; break; } @@ -509,7 +509,7 @@ RingHash::RingHashSubchannelList::Ring::Ring( std::ceil(min_normalized_weight * min_ring_size) / min_normalized_weight, static_cast(max_ring_size)); // Reserve memory for the entire ring up front. - const size_t ring_size = std::ceil(scale); + const uint64_t ring_size = std::ceil(scale); ring_.reserve(ring_size); // Populate the hash ring by walking through the (host, weight) pairs in // normalized_host_weights, and generating (scale * weight) hashes for each diff --git a/src/core/plugin_registry/grpc_plugin_registry.cc b/src/core/plugin_registry/grpc_plugin_registry.cc index 79dd42722e2..8aac81a4884 100644 --- a/src/core/plugin_registry/grpc_plugin_registry.cc +++ b/src/core/plugin_registry/grpc_plugin_registry.cc @@ -62,7 +62,6 @@ extern void RegisterPickFirstLbPolicy(CoreConfiguration::Builder* builder); extern void RegisterRoundRobinLbPolicy(CoreConfiguration::Builder* builder); extern void RegisterWeightedRoundRobinLbPolicy( CoreConfiguration::Builder* builder); -extern void RegisterRingHashLbPolicy(CoreConfiguration::Builder* builder); extern void RegisterHttpProxyMapper(CoreConfiguration::Builder* builder); #ifndef GRPC_NO_RLS extern void RegisterRlsLbPolicy(CoreConfiguration::Builder* builder); @@ -85,7 +84,6 @@ void BuildCoreConfiguration(CoreConfiguration::Builder* builder) { RegisterPickFirstLbPolicy(builder); RegisterRoundRobinLbPolicy(builder); RegisterWeightedRoundRobinLbPolicy(builder); - RegisterRingHashLbPolicy(builder); BuildClientChannelConfiguration(builder); SecurityRegisterHandshakerFactories(builder); RegisterClientAuthorityFilter(builder); diff --git a/src/core/plugin_registry/grpc_plugin_registry_extra.cc b/src/core/plugin_registry/grpc_plugin_registry_extra.cc index ce94a2809bc..43a98d8bb35 100644 --- a/src/core/plugin_registry/grpc_plugin_registry_extra.cc +++ b/src/core/plugin_registry/grpc_plugin_registry_extra.cc @@ -39,6 +39,7 @@ extern void RegisterXdsClusterResolverLbPolicy( extern void RegisterXdsOverrideHostLbPolicy( CoreConfiguration::Builder* builder); extern void RegisterXdsWrrLocalityLbPolicy(CoreConfiguration::Builder* builder); +extern void RegisterRingHashLbPolicy(CoreConfiguration::Builder* builder); extern void RegisterFileWatcherCertificateProvider( CoreConfiguration::Builder* builder); #endif @@ -60,6 +61,7 @@ void RegisterExtraFilters(CoreConfiguration::Builder* builder) { RegisterXdsClusterResolverLbPolicy(builder); RegisterXdsOverrideHostLbPolicy(builder); RegisterXdsWrrLocalityLbPolicy(builder); + RegisterRingHashLbPolicy(builder); RegisterFileWatcherCertificateProvider(builder); #endif }