Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14677 )
Change subject: IMPALA-9137: Blacklist node if a DataStreamService RPC to the node fails ...................................................................... Patch Set 10: (3 comments) http://gerrit.cloudera.org:8080/#/c/14677/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14677/10//COMMIT_MSG@18 PS10, Line 18: the target node. > Does this open up the possibility that a failure in S will > blacklist other healthy nodes? Are there errors in the S->T RPC > that we should ignore for the purpose of blacklisting? That's a good point. I would say, for the purposes of this initial patch, we should differentiate between errors from the rpc layer, i.e. returned by the RpcController, vs. errors with running the rpc itself, i.e. returned in the TransmitDataResponsePB. As a first pass, probably we only want to blacklist stuff when there are errors from the rpc layer, we can think about other errors in later patches (fwiw this is also how the existing blacklisting for failed Exec() rpcs works) http://gerrit.cloudera.org:8080/#/c/14677/10/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/14677/10/be/src/runtime/runtime-state.h@322 PS10, Line 322: AuxErrorInfoPB* aux_error_info() { > Does this ever need to be non-const or can we return a const pointer? Another option to improve encapsulation would be to do what we do with the error map, i.e. make a SetAuxErrorInfo(AuxErrorInfoPB*) http://gerrit.cloudera.org:8080/#/c/14677/10/be/src/runtime/runtime-state.cc File be/src/runtime/runtime-state.cc: http://gerrit.cloudera.org:8080/#/c/14677/10/be/src/runtime/runtime-state.cc@317 PS10, Line 317: NetworkAddressPB* network_addr = new NetworkAddressPB(); > Can you remove the unguarded allocations? You can either replace them with Yeah, strong preference for 'mutable_*' over the other options. -- To view, visit http://gerrit.cloudera.org:8080/14677 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I733cca13847fde43c8ea2ae574d3ae04bd06419c Gerrit-Change-Number: 14677 Gerrit-PatchSet: 10 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Wed, 11 Dec 2019 18:57:18 +0000 Gerrit-HasComments: Yes
