Michael Ho 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 ? 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 ? http://gerrit.cloudera.org:8080/#/c/8950/6/be/src/runtime/data-stream-test.cc@545 PS6, Line 545: FLAGS_port FLAGS_krpc_port ? 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 ? 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. 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. 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 the fix, right ? -- 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: Wed, 24 Jan 2018 19:50:17 +0000 Gerrit-HasComments: Yes
