diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c
index 8645333c8e397612db0d7717b70e786e49eb381b..da2cdfe8e9046056cc72bb3aab7ebbeed3574afc 100644
--- a/src/core/ext/lb_policy/round_robin/round_robin.c
+++ b/src/core/ext/lb_policy/round_robin/round_robin.c
@@ -31,6 +31,34 @@
*
*/
+/** Round Robin Policy.
+ *
+ * This policy keeps:
+ * - A circular list of ready (connected) subchannels, the *readylist*. An empty
+ * readylist consists solely of its root (dummy) node.
+ * - A pointer to the last element picked from the readylist, the *lastpick*.
+ * Initially set to point to the readylist's root.
+ *
+ * Behavior:
+ * - When a subchannel connects, it's *prepended* to the readylist's root node.
+ * Ie, if readylist = A <-> B <-> ROOT <-> C
+ * ^ ^
+ * |____________________|
+ * and subchannel D becomes connected, the addition of D to the readylist
+ * results in readylist = A <-> B <-> D <-> ROOT <-> C
+ * ^ ^
+ * |__________________________|
+ * - When a subchannel disconnects, it's removed from the readylist. If the
+ * subchannel being removed was the most recently picked, the *lastpick*
+ * pointer moves to the removed node's previous element. Note that if the
+ * readylist only had one element, this is still legal, as the lastpick would
+ * point to the dummy root node, for an empty readylist.
+ * - Upon picking, *lastpick* is updated to point to the returned (connected)
+ * subchannel. Note that it's possible that the selected subchannel becomes
+ * disconnected in the interim between the selection and the actual usage of
+ * the subchannel by the caller.
+ */
+
#include <string.h>
#include <grpc/support/alloc.h>
@@ -173,9 +201,7 @@ static void remove_disconnected_sc_locked(round_robin_lb_policy *p,
return;
}
if (node == p->ready_list_last_pick) {
- /* If removing the lastly picked node, reset the last pick pointer to the
- * dummy root of the list */
- p->ready_list_last_pick = &p->ready_list;
+ p->ready_list_last_pick = p->ready_list_last_pick->prev;
}
/* removing last item */
@@ -345,8 +371,8 @@ static int rr_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol,
*target = grpc_subchannel_get_connected_subchannel(selected->subchannel);
if (grpc_lb_round_robin_trace) {
gpr_log(GPR_DEBUG,
- "[RR PICK] TARGET <-- CONNECTED SUBCHANNEL %p (NODE %p)",
- selected->subchannel, selected);
+ "[RR PICK] TARGET <-- CONNECTED SUBCHANNEL %p (NODE %p)", *target,
+ selected);
}
/* only advance the last picked pointer if the selection was used */
advance_last_picked_locked(p);
@@ -526,7 +552,7 @@ static void round_robin_factory_ref(grpc_lb_policy_factory *factory) {}
static void round_robin_factory_unref(grpc_lb_policy_factory *factory) {}
-static grpc_lb_policy *create_round_robin(grpc_exec_ctx *exec_ctx,
+static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx,
grpc_lb_policy_factory *factory,
grpc_lb_policy_args *args) {
GPR_ASSERT(args->addresses != NULL);
@@ -582,7 +608,7 @@ static grpc_lb_policy *create_round_robin(grpc_exec_ctx *exec_ctx,
}
static const grpc_lb_policy_factory_vtable round_robin_factory_vtable = {
- round_robin_factory_ref, round_robin_factory_unref, create_round_robin,
+ round_robin_factory_ref, round_robin_factory_unref, round_robin_create,
"round_robin"};
static grpc_lb_policy_factory round_robin_lb_policy_factory = {
diff --git a/test/core/client_config/lb_policies_test.c b/test/core/client_config/lb_policies_test.c
index 2ec46124a890f49f418934ba414707c95e988138..786f0b39b278d15d9b9ce65492187289621962c1 100644
--- a/test/core/client_config/lb_policies_test.c
+++ b/test/core/client_config/lb_policies_test.c
@@ -640,7 +640,7 @@ static void print_failed_expectations(const int *expected_connection_sequence,
const size_t num_iters) {
size_t i;
for (i = 0; i < num_iters; i++) {
- gpr_log(GPR_ERROR, "FAILURE: Iter, expected, actual:%d (%d, %d)", i,
+ gpr_log(GPR_ERROR, "FAILURE: Iter (expected, actual): %d (%d, %d)", i,
expected_connection_sequence[i % expected_seq_length],
actual_connection_sequence[i]);
}
@@ -664,8 +664,6 @@ static void verify_vanilla_round_robin(const servers_fixture *f,
const int actual = actual_connection_sequence[i];
const int expected = expected_connection_sequence[i % expected_seq_length];
if (actual != expected) {
- gpr_log(GPR_ERROR, "FAILURE: expected %d, actual %d at iter %d", expected,
- actual, i);
print_failed_expectations(expected_connection_sequence,
actual_connection_sequence, expected_seq_length,
num_iters);
@@ -692,24 +690,21 @@ static void verify_vanishing_floor_round_robin(
memcpy(expected_connection_sequence, actual_connection_sequence + 2,
expected_seq_length * sizeof(int));
- /* first three elements of the sequence should be [<1st>, -1] */
- if (actual_connection_sequence[0] != expected_connection_sequence[0]) {
- gpr_log(GPR_ERROR, "FAILURE: expected %d, actual %d at iter %d",
- expected_connection_sequence[0], actual_connection_sequence[0], 0);
- print_failed_expectations(expected_connection_sequence,
- actual_connection_sequence, expected_seq_length,
- 1u);
- abort();
- }
-
+ /* first two elements of the sequence should be [0 (1st server), -1 (failure)]
+ */
+ GPR_ASSERT(actual_connection_sequence[0] == 0);
GPR_ASSERT(actual_connection_sequence[1] == -1);
+ /* the next two element must be [3, 0], repeating from that point: the 3 is
+ * brought forth by servers 1 and 2 disappearing after the intial pick of 0 */
+ GPR_ASSERT(actual_connection_sequence[2] == 3);
+ GPR_ASSERT(actual_connection_sequence[3] == 0);
+
+ /* make sure that the expectation obliges */
for (i = 2; i < num_iters; i++) {
const int actual = actual_connection_sequence[i];
const int expected = expected_connection_sequence[i % expected_seq_length];
if (actual != expected) {
- gpr_log(GPR_ERROR, "FAILURE: expected %d, actual %d at iter %d", expected,
- actual, i);
print_failed_expectations(expected_connection_sequence,
actual_connection_sequence, expected_seq_length,
num_iters);
@@ -757,8 +752,6 @@ static void verify_partial_carnage_round_robin(
const int actual = actual_connection_sequence[i];
const int expected = expected_connection_sequence[i % expected_seq_length];
if (actual != expected) {
- gpr_log(GPR_ERROR, "FAILURE: expected %d, actual %d at iter %d", expected,
- actual, i);
print_failed_expectations(expected_connection_sequence,
actual_connection_sequence, expected_seq_length,
num_iters);
@@ -856,8 +849,6 @@ static void verify_rebirth_round_robin(const servers_fixture *f,
const int expected =
expected_connection_sequence[j++ % expected_seq_length];
if (actual != expected) {
- gpr_log(GPR_ERROR, "FAILURE: expected %d, actual %d at iter %d", expected,
- actual, i);
print_failed_expectations(expected_connection_sequence,
actual_connection_sequence, expected_seq_length,
num_iters);
@@ -887,7 +878,8 @@ int main(int argc, char **argv) {
GPR_ASSERT(grpc_lb_policy_create(&exec_ctx, NULL, NULL) == NULL);
spec = test_spec_create(NUM_ITERS, NUM_SERVERS);
- /* everything is fine, all servers stay up the whole time and life's peachy */
+ /* everything is fine, all servers stay up the whole time and life's peachy
+ */
spec->verifier = verify_vanilla_round_robin;
spec->description = "test_all_server_up";
run_spec(spec);