From c53f8dfbfcb92de66f6e70df7249337eb50dfdfb Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 6 Jul 2018 14:25:43 -0700 Subject: [PATCH 1/6] Fixed ordering in adding pending picks to RR --- .../client_channel/lb_policy/round_robin/round_robin.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc index b1773850653..42e8e88ec95 100644 --- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc +++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc @@ -354,11 +354,11 @@ bool RoundRobin::PickLocked(PickState* pick) { if (DoPickLocked(pick)) return true; } /* no pick currently available. Save for later in list of pending picks */ + pick->next = pending_picks_; + pending_picks_ = pick; if (!started_picking_) { StartPickingLocked(); } - pick->next = pending_picks_; - pending_picks_ = pick; return false; } From d1deaad1be9983e6c90d9022ae0b14fc7414baa2 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Mon, 9 Jul 2018 18:00:54 -0700 Subject: [PATCH 2/6] Added test --- test/cpp/end2end/client_lb_end2end_test.cc | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index feea7c39075..6e06b2523aa 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -539,6 +539,33 @@ TEST_F(ClientLbEnd2endTest, RoundRobin) { EXPECT_EQ("round_robin", channel->GetLoadBalancingPolicyName()); } +TEST_F(ClientLbEnd2endTest, RoundRobinProcessPending) { + StartServers(1); // Single server + auto channel = BuildChannel("round_robin"); + auto stub = BuildStub(channel); + SetNextResolution({servers_[0]->port_}); + WaitForServer(stub, 0, DEBUG_LOCATION); + constexpr int kIterations = 4; + constexpr int kNumThreads = 4; + std::vector threads; + // Create and destroy several channels concurrently, executing an RPC each + // time. This will force the recycling of the underlying (READY) subchannels. + // The RR LB policy of a newly created channel will pick these subchannels in + // READY state. Progress should happen without any transition from this READY + // state. + threads.push_back(std::thread([=]() { + for (int i = 0; i < kNumThreads; ++i) { + auto channel = BuildChannel("round_robin"); + auto stub = BuildStub(channel); + SetNextResolution({servers_[0]->port_}); + for (int i = 0; i < kIterations; ++i) { + CheckRpcSendOk(stub, DEBUG_LOCATION); + } + } + })); + for (auto& thread : threads) thread.join(); +} + TEST_F(ClientLbEnd2endTest, RoundRobinUpdates) { // Start servers and send one RPC per server. const int kNumServers = 3; From e41cae308134addbbbb4daab215cf7b2cd114378 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 10 Jul 2018 13:39:45 -0700 Subject: [PATCH 3/6] Added TODO --- test/cpp/end2end/client_lb_end2end_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index 6e06b2523aa..e582a0f3a42 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -539,6 +539,8 @@ TEST_F(ClientLbEnd2endTest, RoundRobin) { EXPECT_EQ("round_robin", channel->GetLoadBalancingPolicyName()); } +// TODO(juanlishen): Investigate getting rid of threads once +// https://github.com/grpc/grpc/pull/15841 is in. TEST_F(ClientLbEnd2endTest, RoundRobinProcessPending) { StartServers(1); // Single server auto channel = BuildChannel("round_robin"); From 30d39c473878c77ac545d53d13ef9d243edd11ef Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 10 Jul 2018 15:40:02 -0700 Subject: [PATCH 4/6] Clarify new test comment --- test/cpp/end2end/client_lb_end2end_test.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index e582a0f3a42..f22f3f2f2cb 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -551,10 +551,10 @@ TEST_F(ClientLbEnd2endTest, RoundRobinProcessPending) { constexpr int kNumThreads = 4; std::vector threads; // Create and destroy several channels concurrently, executing an RPC each - // time. This will force the recycling of the underlying (READY) subchannels. - // The RR LB policy of a newly created channel will pick these subchannels in - // READY state. Progress should happen without any transition from this READY - // state. + // time. The creation of new channels and their corresponding RR LB policies + // is the important part: new channels/RR policies will pick the subchannels + // in READY state (from a previous RPC against the same target). Progress + // should happen without any transition from this READY state. threads.push_back(std::thread([=]() { for (int i = 0; i < kNumThreads; ++i) { auto channel = BuildChannel("round_robin"); From 50409fea277bfcb93d4c192f6fe6c295c5962558 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 10 Jul 2018 16:10:28 -0700 Subject: [PATCH 5/6] Simplified the test significantly. No more threads --- test/cpp/end2end/client_lb_end2end_test.cc | 32 ++++++++-------------- 1 file changed, 11 insertions(+), 21 deletions(-) diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index f22f3f2f2cb..8fe47db1724 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -539,33 +539,23 @@ TEST_F(ClientLbEnd2endTest, RoundRobin) { EXPECT_EQ("round_robin", channel->GetLoadBalancingPolicyName()); } -// TODO(juanlishen): Investigate getting rid of threads once -// https://github.com/grpc/grpc/pull/15841 is in. TEST_F(ClientLbEnd2endTest, RoundRobinProcessPending) { StartServers(1); // Single server auto channel = BuildChannel("round_robin"); auto stub = BuildStub(channel); SetNextResolution({servers_[0]->port_}); WaitForServer(stub, 0, DEBUG_LOCATION); - constexpr int kIterations = 4; - constexpr int kNumThreads = 4; - std::vector threads; - // Create and destroy several channels concurrently, executing an RPC each - // time. The creation of new channels and their corresponding RR LB policies - // is the important part: new channels/RR policies will pick the subchannels - // in READY state (from a previous RPC against the same target). Progress - // should happen without any transition from this READY state. - threads.push_back(std::thread([=]() { - for (int i = 0; i < kNumThreads; ++i) { - auto channel = BuildChannel("round_robin"); - auto stub = BuildStub(channel); - SetNextResolution({servers_[0]->port_}); - for (int i = 0; i < kIterations; ++i) { - CheckRpcSendOk(stub, DEBUG_LOCATION); - } - } - })); - for (auto& thread : threads) thread.join(); + // Create a new channel and its corresponding RR LB policy, which will pick + // the subchannels in READY state from a the previous RPC against the same + // target (even if it happened over a different channel, because subchannels + // are globally reused). Progress should happen without any transition from + // this READY state. + { + auto second_channel = BuildChannel("round_robin"); + auto second_stub = BuildStub(second_channel); + SetNextResolution({servers_[0]->port_}); + CheckRpcSendOk(second_stub, DEBUG_LOCATION); + } } TEST_F(ClientLbEnd2endTest, RoundRobinUpdates) { From 8427571d0602899591405b6fd4b7e9da96e47b14 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 10 Jul 2018 18:52:01 -0700 Subject: [PATCH 6/6] fix test comment and small code tweak --- test/cpp/end2end/client_lb_end2end_test.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/test/cpp/end2end/client_lb_end2end_test.cc b/test/cpp/end2end/client_lb_end2end_test.cc index 8fe47db1724..8896fc6cae7 100644 --- a/test/cpp/end2end/client_lb_end2end_test.cc +++ b/test/cpp/end2end/client_lb_end2end_test.cc @@ -546,16 +546,14 @@ TEST_F(ClientLbEnd2endTest, RoundRobinProcessPending) { SetNextResolution({servers_[0]->port_}); WaitForServer(stub, 0, DEBUG_LOCATION); // Create a new channel and its corresponding RR LB policy, which will pick - // the subchannels in READY state from a the previous RPC against the same + // the subchannels in READY state from the previous RPC against the same // target (even if it happened over a different channel, because subchannels // are globally reused). Progress should happen without any transition from // this READY state. - { - auto second_channel = BuildChannel("round_robin"); - auto second_stub = BuildStub(second_channel); - SetNextResolution({servers_[0]->port_}); - CheckRpcSendOk(second_stub, DEBUG_LOCATION); - } + auto second_channel = BuildChannel("round_robin"); + auto second_stub = BuildStub(second_channel); + SetNextResolution({servers_[0]->port_}); + CheckRpcSendOk(second_stub, DEBUG_LOCATION); } TEST_F(ClientLbEnd2endTest, RoundRobinUpdates) {