You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Ho (Code Review)" <ge...@cloudera.org> on 2019/06/03 23:30:48 UTC
[Impala-ASF-CR] IMPALA-8143: Enhance DoRpcWithRetry().
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 <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 03 Jun 2019 23:30:48 +0000
Gerrit-HasComments: Yes