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: (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: ::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); : > OK. I see. So, it's pattern matching against the class name to pass the rig We can do that, but then the test output shows that the test has run successfully against the option that we wanted to skip. I'm okay with doing that too, but I just thought that this would be clearer to read from the output. 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 > Are we allowed to run with more than one switches ? I wonder if it makes se We could iterate through different config values like the link above and run tests, but that would mean we would need to run all the tests through the different config values, which is unnecessary. The other reason for specializing this class is to run it only with KRPC. 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: }; > Brief comments about what this class is for. Done http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@282 PS4, Line 282: scoped_ptr<RowBatch> > // Only used for KRPC. Not owned. Done http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@283 PS4, Line 283: _val_; > Should this be initialized to nullptr in ctor or here ? Same for stream_mgr I set them both to nullptr here. http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@550 PS4, Line 550: > nit: extra blank line Done http://gerrit.cloudera.org:8080/#/c/8950/4/be/src/runtime/data-stream-test.cc@799 PS4, Line 799: // the senders' payloads reach the receiver before the receiver is setup. Once the > It'd be nice to add a brief description of this test to explain how it exer 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: 5 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, 18 Jan 2018 21:40:34 +0000 Gerrit-HasComments: Yes
