Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8950 )
Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr ...................................................................... Patch Set 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@138 PS3, Line 138: TransmitDataResponsePB* response, RpcContext* rpc_context) { nit: indent off. http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@143 PS3, Line 143: EndDataStreamResponsePB* response, RpcContext* rpc_context) { nit: indent off. http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@174 PS3, Line 174: if (GetParam() == USE_THRIFT) { : FLAGS_use_krpc = false; : } else { : FLAGS_use_krpc = true; : } FLAGS_use_krpc = GetParam() == USE_KRPC; http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@194 PS3, Line 194: stream_mgr_ = ExecEnv::GetInstance()->stream_mgr(); Can't this line be factored out ? http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@557 PS3, Line 557: FLAGS_datastream_service_num_svc_threads Do we actually customize this flag ? if not, seems simpler to just start some fixed number of threads. http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@560 PS3, Line 560: FLAGS_datastream_service_queue_depth Same comment about this flag. http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@583 PS3, Line 583: info.thread_handle nit: indent off. http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@601 PS3, Line 601: void ThriftSender This seems to be mostly identical to KrpcSender. Can we refactor the code to share them somehow (e.g. pass the sender as argument) ? http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@689 PS3, Line 689: class DataStreamTestForImpala2931 : public DataStreamTest { : protected: : virtual void SetUp() { : DataStreamTest::SetUp(); : } : : virtual void TearDown() { : DataStreamTest::TearDown(); : } : }; What's special about this class ? Isn't it mostly the same as DataStreamtest ? http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/krpc-data-stream-mgr.cc File be/src/runtime/krpc-data-stream-mgr.cc: http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/krpc-data-stream-mgr.cc@113 PS3, Line 113: // Move the early senders list here so that we can drop 'lock_'. Please also add a comment that we must drop the lock_ before processing the early senders or we may run into a deadlock. -- To view, visit http://gerrit.cloudera.org:8080/8950 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib7d1a8f12a4821092ca61ccc8a6f20c0404d56c7 Gerrit-Change-Number: 8950 Gerrit-PatchSet: 3 Gerrit-Owner: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Comment-Date: Fri, 12 Jan 2018 22:18:26 +0000 Gerrit-HasComments: Yes
