Joe McDonnell 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: (3 comments) http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/exec/partitioned-hash-join-builder.h File be/src/exec/partitioned-hash-join-builder.h: http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/exec/partitioned-hash-join-builder.h@544 PS4, Line 544: CyclicBarrier* probe_barrier() const { return probe_barrier_.get(); } I would rather not expose the CyclicBarrier directly. Maybe we can have an Unregister() method on PhjBuilder that calls Unregister() on the barrier? http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier.h File be/src/util/cyclic-barrier.h: http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier.h@36 PS4, Line 36: CyclicBarrier(int num_threads); For barriers that use unregister, it would nice to enforce that every thread unregisters. That can be enforced by checking that num_threads_==0 in the destructor. For barriers that don't use unregister, we can skip that. http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier.cc File be/src/util/cyclic-barrier.cc: http://gerrit.cloudera.org:8080/#/c/20179/4/be/src/util/cyclic-barrier.cc@43 PS4, Line 43: --num_threads_; DCHECK that num_threads_ > 0. -- 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 18:21:53 +0000 Gerrit-HasComments: Yes
