Sailesh Mukil 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. Done 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. Done 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; Done 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 ? Done 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 so No we don't. Done. 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. Done 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. Done 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 t Yes it can be done by making the sender object as a DataSink type and casting where necessary. Done. 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 DataStreamtes It is, but we only want to run this test for Thrift and not for KRPC. So if you look below in L716, we only pass in the USE_THRIFT flag for DataStreamTestForImpala2931. 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 Done -- 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-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Comment-Date: Tue, 16 Jan 2018 23:25:40 +0000 Gerrit-HasComments: Yes
