Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20179 )
Change subject: IMPALA-12233: Fixed PHJ hanging caused by cyclic barrier ...................................................................... Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/exec/partitioned-hash-join-node.cc File be/src/exec/partitioned-hash-join-node.cc: http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/exec/partitioned-hash-join-node.cc@289 PS4, Line 289: if (builder_ && builder_->probe_barrier()) builder_->probe_barrier()->Unregister(); I guess the number of remaining transitions for other threads could be unpredictable? A single wait here wouldn't be sufficient, right? http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier-test.cc File be/src/util/cyclic-barrier-test.cc: http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier-test.cc@168 PS4, Line 168: // All threads except one should join the barrier, then get cancelled. This description doesn't quite match up with how I read it. Maybe "All threads except one should join the barrier or unregister." I'm not sure what "get cancelled" means. http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier-test.cc@190 PS4, Line 190: EXPECT_EQ(0, waits_complete.Load()) << "Threads should have returned."; This should be "Threads should not have returned." or something similar. http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier-test.cc@213 PS4, Line 213: // All threads except one should join the barrier, then get cancelled. nit: same as above. http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier-test.cc@235 PS4, Line 235: EXPECT_EQ(0, waits_complete.Load()) << "Threads should have returned."; nit: same as above. http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier-test.cc@252 PS4, Line 252: EXPECT_EQ(NUM_OF_UNREGISTERING_THREADS, unregisters_complete.Load()) nit: You already checked this at line 236, seems redundant. http://gerrit.cloudera.org:8080/#/c/20179/4/testdata/workloads/functional-query/queries/QueryTest/joins_mt_dop.test File testdata/workloads/functional-query/queries/QueryTest/joins_mt_dop.test: http://gerrit.cloudera.org:8080/#/c/20179/4/testdata/workloads/functional-query/queries/QueryTest/joins_mt_dop.test@54 PS4, Line 54: set num_nodes=3; Why set num_nodes=3? Doesn't seem to be necessary. -- To view, visit http://gerrit.cloudera.org:8080/20179 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8be75c7ce99c015964c8bbb547539e6619ba4f9b Gerrit-Change-Number: 20179 Gerrit-PatchSet: 4 Gerrit-Owner: Gergely Fürnstáhl <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Thu, 13 Jul 2023 17:57:09 +0000 Gerrit-HasComments: Yes
