Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/8950 )
Change subject: IMPALA-6346: Potential deadlock in KrpcDataStreamMgr ...................................................................... Patch Set 5: (2 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: ::testing::Values(USE_KRPC)); : : TEST_P(DataStreamTest, UnknownSenderSmallResult) { : // 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); : > I find this class specialization a bit misleading given it's essentialy the I'll decide which one to do based on your thoughts on my comment below. http://gerrit.cloudera.org:8080/#/c/8950/3/be/src/runtime/data-stream-test.cc@703 PS3, Line 703: TEST_P(DataStreamTest, UnknownSenderLargeResult) { : // case 2: query result requires multiple buffers > It seems rather hard to reuse the code for exercising other cases in the KR So this class is not reusable for other tests. It's hard to make a class that can be used for KRPC only where for each different test we would want a different flag configuration. The reason is that a lot of these flags get picked up in ExecEnv creation time, which happens in the SetUp() phase. The idea behind gtest's SetUp() and TearDown() helpers was to ensure that the same setup and tear down happens for each test. But in our case, we're trying to configure the SetUp and TearDown portion itself, which seems like an anti-pattern for gtest. In order to reach maximum flexibility, we would need to move all the SetUp() and TearDown() code into the tests itself and set the flags we require before calling SetUp(). In that case, we can have a DataStreamTestForThriftOnly and a DataStreamTestForKRPCOnly. What do you think? -- 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: 5 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: Fri, 19 Jan 2018 18:55:34 +0000 Gerrit-HasComments: Yes
