Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12002 )
Change subject: KUDU-683: unify C++ client-to-master retry logic ...................................................................... Patch Set 6: (19 comments) http://gerrit.cloudera.org:8080/#/c/12002/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12002/5//COMMIT_MSG@27 PS5, Line 27: - SyncLeaderMasterRpcs used exponential backoff when retrying RPCs, but : the existing RpcRetrier implemented its own backoff. To maintain : this, I've piped down an enum from Rpc to RpcRetrier to determine : which backoff strategy to use. : - The new template and most other existing Rpc im > I'm not convinced that both kinds of backoff are warranted, but let's prete It's tricky doing it with a template parameter without inheritance. In some form, I think the templates would need to refer to some single Rpc or RpcRetrier base class, since the original classes point to each other. I've gone about piping down an enum to the constructors. http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/client-internal.cc@162 PS5, Line 162: const ReqClass& req, > nit: extra space. Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h File src/kudu/client/master_proxy_rpc.h: http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@51 PS5, Line 51: // reconnection to the master(s). > Doc the class too. Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@53 PS5, Line 53: class AsyncLeaderMasterRpc : public rpc::Rpc { > Lots of args, please doc. Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@55 PS5, Line 55: // The input 'client' will be used to call the asynchonous master proxy : // function 'func' on the currently-k > Can the AsyncLeaderMasterRpc declare its own req/resp members and provide a It's important so this can be used to service proxy requests. I suppose you could `*resp = rpc.resp()` if anything, but this seems more natural, and more flexible (e.g. LookupRpc points the AsyncLeaderMasterRpc at internal members, and SyncLeaderMasterRpc can utilize the proxy input parameters). http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@95 PS5, Line 95: RPC controller > nit: doc what the 'status' out param means? Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.h@122 PS5, Line 122: const Stat > nit: doc what this is for? Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc File src/kudu/client/master_proxy_rpc.cc: http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc@92 PS5, Line 92: > nit: extra space. Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc@196 PS5, Line 196: table_retrier = this-> > readability nit: here and below, maybe store the pointer to the retrier int Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc@217 PS5, Line 217: n't yet been updated wit > readability nit: since the multi-master property is a not dynamic one, mayb Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/master_proxy_rpc.cc@281 PS5, Line 281: > For consistency, can you format all of these the same way? Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc@20 PS5, Line 20: cstdint> > nit: <cstdint> ? Ok, but I'm pretty sure this is what IWYU wanted. http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc@82 PS5, Line 82: using tserver::TabletServerServiceProxy; > warning: using decl 'Messenger' is unused [misc-unused-using-decls] Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc@563 PS5, Line 563: void Sen > nit here and below for virtual methods marked with 'override': drop 'virtua Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/client/meta_cache.cc@731 PS5, Line 731: // One or more of the tablets is not running. Retry after some time. > Can you explain a bit more here why RetryOrReconnectIfNecessary() cannot ha Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/retriable_rpc.h File src/kudu/rpc/retriable_rpc.h: http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/retriable_rpc.h@24 PS5, Line 24: #include "kudu/rpc/messenger.h" > warning: #includes are not sorted properly [llvm-include-order] Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/retriable_rpc.h@51 PS5, Line 51: // TODO(awong): there might be room to merge part of this with retry logic in > Hard to reason about RetriableRpc vis a vis RetryingRpc. Done http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/rpc.h File src/kudu/rpc/rpc.h: http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/rpc.h@115 PS5, Line 115: > Could we merge the retrier into the Rpc class? I think the decomposition ha While I agree that the RpcRetrier abstraction made parts of this change not ideal, I'm going to punt on this, given how deeply-plumbed it is already. I'd like to keep this change targeted to what it's already got. Though good point about HandleResponse(); I've taken it out it since it was redundant with logic in AsyncLeaderMasterRpc (and elsewhere) and was only used in one other place, AFAICT. It was kind of ruining the "new" way of life anyway, where handling is done in the Rpc implementations, rather than the base Rpc class. http://gerrit.cloudera.org:8080/#/c/12002/5/src/kudu/rpc/rpc.h@229 PS5, Line 229: DISALLOW_COPY_AND_ASSIGN(Rpc); > It's tough to see the distinction between this class and Rpc. Both expose a Yeah, I'd stubbornly wanted to templatize everything, and couldn't get around it without this abstract class (unless I went about tearing up further Rpc and RpcRetrier). I've restored the original class hierarchy and piped down an enum to dictate the backoff strategy. -- To view, visit http://gerrit.cloudera.org:8080/12002 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2450676da1c723a247c84deb1b895f116173670e Gerrit-Change-Number: 12002 Gerrit-PatchSet: 6 Gerrit-Owner: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 17 Dec 2018 18:10:47 +0000 Gerrit-HasComments: Yes
