Sahil Takiar 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 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/14677/9/be/src/runtime/krpc-data-stream-sender.cc File be/src/runtime/krpc-data-stream-sender.cc: http://gerrit.cloudera.org:8080/#/c/14677/9/be/src/runtime/krpc-data-stream-sender.cc@406 PS9, Line 406: unique_ptr > Not sure that it's necessary to use a unique_ptr and move() here - the Stat Removed the unique_ptr usage. Removed the StatusAuxInfo class entirely. http://gerrit.cloudera.org:8080/#/c/14677/9/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/14677/9/be/src/runtime/runtime-state.h@306 PS9, Line 306: StatusAuxInfo& status_aux_info() { > I guess you're relying on the lock in StatusAuxInfo to make this thread-saf Done. Removed the StatusAuxInfo class entirely. http://gerrit.cloudera.org:8080/#/c/14677/9/be/src/runtime/status-aux-info.h File be/src/runtime/status-aux-info.h: http://gerrit.cloudera.org:8080/#/c/14677/9/be/src/runtime/status-aux-info.h@61 PS9, Line 61: StatusAuxInfoPB* CreateStatusAuxInfoPB() > Seems sort of weird to create a 'new StatusAuxInfoPB()" and then use set_al No longer need this since RuntimeState just uses AuxErrorInfoPB directly. http://gerrit.cloudera.org:8080/#/c/14677/9/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/14677/9/common/protobuf/control_service.proto@155 PS9, Line 155: // Metadata that can be associated with a StatusPB object. > Might be useful to explain this a little more, eg. something like "Used to Done http://gerrit.cloudera.org:8080/#/c/14677/9/common/protobuf/control_service.proto@184 PS9, Line 184: optional StatusAuxInfoPB status_aux_info = 7; > Might make more sense to put this into FragmentInstanceExecStatusPB, since Done. Changed the name to AuxErrorInfoPB as well since FragmentInstanceExecStatusPB doesn't have a StatusPB object. http://gerrit.cloudera.org:8080/#/c/14677/9/common/protobuf/control_service.proto@289 PS9, Line 289: } > ? Done http://gerrit.cloudera.org:8080/#/c/14677/9/tests/custom_cluster/test_blacklist.py File tests/custom_cluster/test_blacklist.py: http://gerrit.cloudera.org:8080/#/c/14677/9/tests/custom_cluster/test_blacklist.py@139 PS9, Line 139: e > flake8: E501 line too long (99 > 90 characters) Done -- 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: 9 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Mon, 09 Dec 2019 19:54:31 +0000 Gerrit-HasComments: Yes
