fix for code review comments

src/brpc/policy/weighted_round_robin_load_balancer.cpp

@@ -14,6 +14,8 @@
 
 // Authors: Daojin Cai (caidaojin@qiyi.com)
 
+#include <algorithm>
+
 #include "butil/fast_rand.h"
 #include "brpc/socket.h"
 #include "brpc/policy/weighted_round_robin_load_balancer.h"
@@ -22,6 +24,11 @@
 namespace brpc {
 namespace policy {
 
+const std::vector<uint32_t> prime_stride = {
+2,3,5,11,17,29,47,71,107,137,163,251,307,379,569,683,
+857,1289,1543,1949,2617,2927,3407,4391,6599,9901,14867,
+22303,33457,50207,75323,112997,169501,254257,381389,572087};
+
 bool IsCoprime(uint32_t num1, uint32_t num2) {
     uint32_t temp;
     if (num1 < num2) {
@@ -64,9 +71,9 @@ bool WeightedRoundRobinLoadBalancer::Remove(Servers& bg, const ServerId& id) {
     auto iter = bg.server_map.find(id.id);
     if (iter != bg.server_map.end()) {
         const size_t index = iter->second;
-        bg.weight_sum -= bg.server_list[index].second;
+        bg.weight_sum -= bg.server_list[index].weight;
         bg.server_list[index] = bg.server_list.back();
-        bg.server_map[bg.server_list[index].first] = index;
+        bg.server_map[bg.server_list[index].id] = index;
         bg.server_list.pop_back();
         bg.server_map.erase(iter);
         return true;
@@ -137,13 +144,13 @@ int WeightedRoundRobinLoadBalancer::SelectServer(const SelectIn& in, SelectOut*
     tls.position %= s->server_list.size();
     // Check whether remain server was removed from server list.
     if (tls.HasRemainServer() && 
-        s->server_map.find(tls.remain_server.first) == s->server_map.end()) {
+        tls.remain_server.id != s->server_list[tls.position].id) {
         tls.ResetRemainServer();
     }
-    for ( uint32_t i = 0; i != tls.stride; ++i) {
-        int64_t best = GetBestServer(s->server_list, tls);
-        if (!ExcludedServers::IsExcluded(in.excluded, best)
-            && Socket::Address(best, out->ptr) == 0
+    for (uint32_t i = 0; i != tls.stride; ++i) {
+        int64_t server_id = GetServerInNextStride(s->server_list, tls);
+        if (!ExcludedServers::IsExcluded(in.excluded, server_id)
+            && Socket::Address(server_id, out->ptr) == 0
             && !(*out->ptr)->IsLogOff()) {
             return 0;
         }
@@ -151,35 +158,30 @@ int WeightedRoundRobinLoadBalancer::SelectServer(const SelectIn& in, SelectOut*
     return EHOSTDOWN;
 }
 
-int64_t WeightedRoundRobinLoadBalancer::GetBestServer(
-        const std::vector<std::pair<SocketId, int>>& server_list, TLS& tls) {
+int64_t WeightedRoundRobinLoadBalancer::GetServerInNextStride(
+        const std::vector<Server>& server_list, TLS& tls) {
     int64_t final_server = -1;
     int stride = tls.stride;
-    int weight = 0;
-    while (stride > 0) {
     if (tls.HasRemainServer()) {
-          weight = tls.remain_server.second;
-          if (weight <= stride) {
-              tls.ResetRemainServer();
-          } else {
-              tls.remain_server.second -= stride;
-          }
+        final_server = tls.remain_server.id;
+        if (tls.remain_server.weight > stride) {
+            tls.remain_server.weight -= stride;
+            return final_server;
         } else {
-          weight = server_list[tls.position].second;
-          if (weight > stride) {
-              tls.SetRemainServer(server_list[tls.position].first, 
-                                  weight - stride);
-          }
+            stride -= tls.remain_server.weight;
+            tls.ResetRemainServer();
             tls.UpdatePosition(server_list.size()); 
         }
-      stride -= weight;
     }
-    if (tls.HasRemainServer()) {
-        final_server = tls.remain_server.first;
-    } else {
-        size_t index = tls.position == 0 ? server_list.size() - 1 
-                                      : tls.position - 1;
-        final_server = server_list[index].first;
+    while (stride > 0) {
+        final_server = server_list[tls.position].id;
+        if (server_list[tls.position].weight > stride) {
+            tls.SetRemainServer(server_list[tls.position].id, 
+                                server_list[tls.position].weight - stride);
+            return final_server;
+        }
+        stride -= server_list[tls.position].weight;
+        tls.UpdatePosition(server_list.size()); 
     }
     return final_server;
 }
@@ -187,12 +189,14 @@ int64_t WeightedRoundRobinLoadBalancer::GetBestServer(
 uint32_t WeightedRoundRobinLoadBalancer::GetStride(
     const uint32_t weight_sum, const uint32_t num) {
     uint32_t average_weight = weight_sum / num;
-    // The stride is the first number which is greater than or equal to 
-    // average weight and coprime to weight_sum.
-    while (!IsCoprime(weight_sum, average_weight)) {
-       ++average_weight;
-    }
-    return average_weight;  
+    auto iter = std::lower_bound(prime_stride.begin(), prime_stride.end(), 
+                                 average_weight);
+    while (iter != prime_stride.end()
+           && !IsCoprime(weight_sum, *iter)) {
+        ++iter;
+    }
+    CHECK(iter != prime_stride.end()) << "Failed to get stride";
+    return *iter > weight_sum ? *iter % weight_sum : *iter;  
 }
 
 LoadBalancer* WeightedRoundRobinLoadBalancer::New() const {
@@ -216,7 +220,7 @@ void WeightedRoundRobinLoadBalancer::Describe(
     } else {
         os << "n=" << s->server_list.size() << ':';
         for (const auto& server : s->server_list) {
-            os << ' ' << server.first << '(' << server.second << ')';
+            os << ' ' << server.id << '(' << server.weight << ')';
         }
     }
     os << '}';

src/brpc/policy/weighted_round_robin_load_balancer.h

@@ -39,27 +39,31 @@ public:
     void Describe(std::ostream&, const DescribeOptions& options);
 
 private:
+    struct Server {
+        Server(SocketId s_id = 0, int s_w = 0): id(s_id), weight(s_w) {}
+        SocketId id;
+        int weight;
+    };
     struct Servers {
         // The value is configured weight for each server.
-        std::vector<std::pair<SocketId, int>> server_list;
+        std::vector<Server> server_list;
         // The value is the index of the server in "server_list".
         std::map<SocketId, size_t> server_map;
         uint32_t weight_sum = 0;
     };
     struct TLS {
-        TLS(): remain_server(0, 0) { }
         uint32_t position = 0;
         uint32_t stride = 0;
-        std::pair<SocketId, int> remain_server;
+        Server remain_server;
         bool HasRemainServer() const {
-            return remain_server.second != 0;
+            return remain_server.weight != 0;
         }
         void SetRemainServer(const SocketId id, const int weight) {
-            remain_server.first = id;
-            remain_server.second = weight;
+            remain_server.id = id;
+            remain_server.weight = weight;
         }
         void ResetRemainServer() {
-            remain_server.second = 0;
+            remain_server.weight = 0;
         }
         void UpdatePosition(const uint32_t size) {
             ++position;
@@ -84,8 +88,7 @@ private:
     static bool Remove(Servers& bg, const ServerId& id);
     static size_t BatchAdd(Servers& bg, const std::vector<ServerId>& servers);
     static size_t BatchRemove(Servers& bg, const std::vector<ServerId>& servers);
-    static int64_t GetBestServer(
-                       const std::vector<std::pair<SocketId, int>>& server_list,
+    static int64_t GetServerInNextStride(const std::vector<Server>& server_list,
                                          TLS& tls);
     // Get a reasonable stride according to weights configured of servers. 
     static uint32_t GetStride(const uint32_t weight_sum, const uint32_t num);