Copilot commented on code in PR #3502:
URL: https://github.com/apache/brpc/pull/3502#discussion_r3885903941
##########
src/brpc/policy/consistent_hashing_load_balancer.cpp:
##########
@@ -395,16 +408,223 @@ bool ConsistentHashingLoadBalancer::SetParameters(const
butil::StringPiece& para
LOG(ERROR) << "Empty value for " << sp.key() << " in lb parameter";
return false;
}
- if (sp.key() == "replicas") {
- if (!butil::StringToSizeT(sp.value(), &_num_replicas)) {
- return false;
+ if (!SetParameter(sp.key(), sp.value())) {
+ return false;
+ }
+ }
+ return true;
+}
+
+bool ConsistentHashingLoadBalancer::SetParameter(
+ const butil::StringPiece& key, const butil::StringPiece& value) {
+ if (key == "replicas") {
+ return butil::StringToSizeT(value, &_num_replicas);
+ }
+ LOG(ERROR) << "Failed to set this unknown parameters " << key << '=' <<
value;
+ return true;
Review Comment:
`SetParameter` logs an error for unknown lb parameters but still returns
true, which makes `SetParameters` succeed and `New()` create a load balancer
even though parameter parsing reported failure. This is inconsistent with other
load balancers (e.g. p2c) that reject unknown parameters and return false.
##########
docs/en/client.md:
##########
@@ -278,10 +278,16 @@ Need to set Controller.set_request_code() before RPC
otherwise the RPC will fail
Do distinguish "key" and "attributes" of the request. Don't compute
request_code by full content of the request just for quick. Minor change in
attributes may result in totally different hash code and change destination
dramatically. Another cause is padding, for example: `struct Foo { int32_t a;
int64_t b; }` has a 4-byte undefined gap between `a` and `b` on 64-bit
machines, result of `hash(&foo, sizeof(foo))` is undefined. Fields need to be
packed or serialized before hashing.
+Number of virtual nodes per server defaults to -chash_num_replicas(default
100) and can be overridden per channel: `c_murmurhash:replicas=300`.
Review Comment:
Minor formatting: add a space before the parenthesized default value for
readability/consistency (e.g. `-flag (default ...)`).
##########
test/brpc_ch_bounded_load_balancer_unittest.cpp:
##########
@@ -0,0 +1,347 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you 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 <cmath>
+#include <cstdlib>
+#include <cstring>
+#include <map>
+#include <sstream>
+#include <vector>
+#include <gflags/gflags.h>
+#include <gtest/gtest.h>
+#include "butil/macros.h"
+#include "brpc/socket.h"
+#include "brpc/excluded_servers.h"
+#include "brpc/policy/consistent_hashing_load_balancer.h"
+#include "brpc/policy/hasher.h"
+
+namespace {
+
+brpc::ServerId CreateServer(const char* addr, const char* tag = "") {
+ butil::EndPoint point;
+ EXPECT_EQ(0, str2endpoint(addr, &point));
+ brpc::ServerId id(8888);
+ brpc::SocketOptions options;
+ options.remote_side = point;
+ EXPECT_EQ(0, brpc::Socket::Create(options, &id.id));
+ id.tag = tag;
+ return id;
+}
+
+void DestroyServers(const std::vector<brpc::ServerId>& ids) {
+ for (size_t i = 0; i < ids.size(); ++i) {
+ brpc::Socket::SetFailed(ids[i].id);
+ }
+}
+
+brpc::LoadBalancer::SelectIn MakeInput(uint64_t code,
+ bool changable_weights = true) {
+ brpc::LoadBalancer::SelectIn in = { 0, changable_weights, true, code,
nullptr };
+ return in;
+}
+
+int64_t TotalInflightOf(brpc::LoadBalancer* lb) {
+ std::ostringstream os;
+ brpc::DescribeOptions opt;
+ opt.verbose = true;
+ lb->Describe(os, opt);
+ const std::string desc = os.str();
+ const size_t pos = desc.find("total_inflight=");
+ EXPECT_NE(std::string::npos, pos) << desc;
+ return strtoll(desc.c_str() + pos + strlen("total_inflight="), nullptr,
10);
+}
+
+class CHBoundedLoadTest : public testing::Test {};
+
+TEST_F(CHBoundedLoadTest, load_factor_validation) {
+ brpc::policy::ConsistentHashingBoundedLoadBalancer lb(
+ brpc::policy::CONS_HASH_LB_MURMUR3);
+ ASSERT_EQ(nullptr, lb.New("load_factor=1.0"));
+ ASSERT_EQ(nullptr, lb.New("load_factor=0.5"));
+ ASSERT_EQ(nullptr, lb.New("load_factor=abc"));
+ brpc::LoadBalancer* valid = lb.New("load_factor=1.5");
+ ASSERT_TRUE(valid != nullptr);
+ valid->Destroy();
+
+ ASSERT_EQ("", GFLAGS_NAMESPACE::SetCommandLineOption(
+ "chash_bounded_load_factor", "0.9"));
+ ASSERT_EQ("", GFLAGS_NAMESPACE::SetCommandLineOption(
+ "chash_bounded_load_factor", "1.0"));
+ ASSERT_NE("", GFLAGS_NAMESPACE::SetCommandLineOption(
+ "chash_bounded_load_factor", "1.25"));
Review Comment:
This test mutates a global gflag (`chash_bounded_load_factor`) without any
RAII restoration. Other tests in this repo use `GFLAGS_NAMESPACE::FlagSaver` or
a scoped helper to prevent leaking flag changes across tests; doing the same
here will keep the test order-independent even if the flag’s default changes
later.
##########
docs/en/client.md:
##########
@@ -278,10 +278,16 @@ Need to set Controller.set_request_code() before RPC
otherwise the RPC will fail
Do distinguish "key" and "attributes" of the request. Don't compute
request_code by full content of the request just for quick. Minor change in
attributes may result in totally different hash code and change destination
dramatically. Another cause is padding, for example: `struct Foo { int32_t a;
int64_t b; }` has a 4-byte undefined gap between `a` and `b` on 64-bit
machines, result of `hash(&foo, sizeof(foo))` is undefined. Fields need to be
packed or serialized before hashing.
+Number of virtual nodes per server defaults to -chash_num_replicas(default
100) and can be overridden per channel: `c_murmurhash:replicas=300`.
+
Check out [Consistent Hashing](consistent_hashing.md) for more details.
Other kind of lb does not need to set Controller.set_request_code(). If
request code is set, it will not be used by lb. For example, lb=rr, and call
Controller.set_request_code(), even if request_code is the same for every
request, lb will balance the requests using the rr policy.
+### c_murmurhash_bl
+
+which is consistent hashing with bounded loads("Consistent Hashing with
Bounded Loads", Mirrokni et al., CACM 2017). The hash ring is identical to
`c_murmurhash`, but each server additionally has a capacity of
`ceil(load_factor * average in-flight requests)`. When the hashed-to server is
at capacity, the request overflows clockwise to the next server on the ring
with spare capacity, so a hot key no longer saturates a single server while
overflowed requests always land on the same ring successors, which keeps caches
effective. The default factor comes from -chash_bounded_load_factor(default
1.25, must be > 1) and can be overridden per channel:
`c_murmurhash_bl:load_factor=1.5`. The `replicas` parameter is supported as in
`c_murmurhash`.
Review Comment:
Minor formatting: missing spaces before parentheses make this harder to read
(e.g. `loads (` and `-flag (default ...)`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
