Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/8950 )
Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr ...................................................................... Patch Set 4: (7 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@689 PS3, Line 689: // starting a sender w/o a corresponding receiver results in an error. No bytes should : // be sent. : // case 1: entire query result fits in single buffer : TUniqueId dummy_id; : GetNextInstanceId(&dummy_id); : StartSender(TPartitionType::UNPARTITIONED, TOTAL_DATA_SIZE + 1024); : JoinSenders(); : EXPECT_EQ(sender_info_[0].status.code(), TErrorCode::DATASTREAM_SENDER_TIMEOUT); : EXPECT_EQ(sender_info_[0].num_bytes_sent, 0); : } > It is, but we only want to run this test for Thrift and not for KRPC. So if OK. I see. So, it's pattern matching against the class name to pass the right parameter. Why cannot we just call GetParam() inside the test function and skip if the test doesn't support say KRPC ? http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@703 PS3, Line 703: GetNextInstanceId(&dummy_id); : StartSender(); Are we allowed to run with more than one switches ? I wonder if it makes sense to iterate through different configuration values for these flags. We can implement it with a nested loop like the following: https://github.com/apache/impala/blob/master/be/src/runtime/bufferpool/buffer-allocator-test.cc#L206-L223 If you agree with the above suggestion, these FLAGS can always be initialized in DataStreamTest::SetUp() and not have to specialize it as a class like this. http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc File be/src/runtime/data-stream-test.cc: http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@130 PS4, Line 130: class ImpalaKRPCTestBackend : public DataStreamServiceIf { Brief comments about what this class is for. http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@282 PS4, Line 282: // Only used for KRPC // Only used for KRPC. Not owned. http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@283 PS4, Line 283: krpc_mgr_ Should this be initialized to nullptr in ctor or here ? Same for stream_mgr_. http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@550 PS4, Line 550: nit: extra blank line http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@799 PS4, Line 799: TEST_P(DataStreamTestForImpala6346, TestNoDeadlock) { It'd be nice to add a brief description of this test to explain how it exercises the path which leads to 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: 4 Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Comment-Date: Wed, 17 Jan 2018 02:23:49 +0000 Gerrit-HasComments: Yes
