Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12672 )
Change subject: IMPALA-8143: Enhance DoRpcWithRetry(). ...................................................................... Patch Set 11: Code-Review+2 (6 comments) LGTM. Please address the nits. http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/impala-service-pool.h File be/src/rpc/impala-service-pool.h: http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/impala-service-pool.h@72 PS11, Line 72: string char* See https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/impala-service-pool.cc File be/src/rpc/impala-service-pool.cc: http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/impala-service-pool.cc@192 PS11, Line 192: !debug_status.ok() UNLIKELY(!debug_status.ok()) http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/rpc-mgr-test.h File be/src/rpc/rpc-mgr-test.h: http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/rpc-mgr-test.h@281 PS11, Line 281: getNumberOfCalls() nit: number_of_calls() or GetNumberOfCalls() http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/rpc-mgr-test.cc File be/src/rpc/rpc-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/rpc-mgr-test.cc@310 PS11, Line 310: NULL nit: nullptr http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/rpc-mgr-test.cc@331 PS11, Line 331: // DoRpcWithRetry will fail 1 in 2^40 times. : ASSERT_TRUE(status.ok()); : } : // There will be no overflows (i.e. service too busy) 1 in 2^40 times. : ASSERT_GT(overflow_metric->GetValue(), 0); :-). Subtle nits: Although they are similar this is referring to the probability of the debug action not kicking in at all in the loop above is: (1/2)^40 times. The other one on line 331 refers to the probability of the debugging action kicking in 40 times in a row. I suppose it's slightly clearer to state in (1/2)^num_rpc_retry_calls and (1/2)^num_retries. Please feel free to ignore as they are rather minor nits. http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/rpc-mgr.h File be/src/rpc/rpc-mgr.h: http://gerrit.cloudera.org:8080/#/c/12672/11/be/src/rpc/rpc-mgr.h@183 PS11, Line 183: /// Pass 'debug_action' to DebugAction() to potentially inject errors. Please add a TODO: TODO: Clean up this interface. Replace the debug action with fault injection in RPC callbacks or other places. -- To view, visit http://gerrit.cloudera.org:8080/12672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia9693151c35e02235665b3c285a48c585973d390 Gerrit-Change-Number: 12672 Gerrit-PatchSet: 11 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Mon, 03 Jun 2019 23:30:48 +0000 Gerrit-HasComments: Yes
