Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8950 )
Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr ...................................................................... Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@77 PS6, Line 77: DEFINE_int32(port, 20001, "port on which to run Impala Thrift based test backend."); > Why don't we just use FLAGS_be_port here ? 'be_port' is only started via ImpalaServer::Start(), which happens from impalad-main.cc. We don't go through that code path. In any case, when we try to start it here, it is unable to bind to the be_port. krpc_port is prepared in ExecEnv::Init(), which we don't call here, since we only call ExecEnv::InitForFeTests(). So the krpc_port is reserved, but the code path that starts the services on it is never called. We can make both these ports work in this test, but that would require quite a lot of rewriting of how and where the ports are bound to and the services on them started. I think for the purposes of this patch, it's simpler to start test backends on a different port. http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@79 PS6, Line 79: DECLARE_int32(datastream_service_num_svc_threads); : DECLARE_int32(datastream_service_queue_depth); > delete ? Done http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@545 PS6, Line 545: FLAGS_port > FLAGS_krpc_port ? See my comment above. http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@609 PS6, Line 609: EXPECT_OK(static_cast<KrpcDataStreamSender*>( : sender.get())->Init(vector<TExpr>({output_exprs}), sink, &state)); > Can this be factored out too ? No it can't since DataSink has its own Init() function. So unless we cast and call, it won't call the correct Init(). http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@658 PS6, Line 658: class DataStreamTestThriftOnly : public DataStreamTest { > Please add a comment what this class is for. Done http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@668 PS6, Line 668: : class DataStreamTestForImpala6346 : public DataStreamTest { > Please add a comment why we need to have a class for this test. Done http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@808 PS6, Line 808: TEST_P(DataStreamTestForImpala6346, TestNoDeadlock) { > Just double checking: this test is able to reproduce the deadlock without t Yes, I confirmed that without the fix, the deadlock occurs. -- 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: 6 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: Thu, 25 Jan 2018 18:35:01 +0000 Gerrit-HasComments: Yes
