Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12968 )
Change subject: IMPALA-8322: Add periodic dirty check of done_ in ThreadTokenAvailableCb ...................................................................... Patch Set 2: Code-Review+1 (4 comments) I'll leave it to Lars or other reviewers to do the +2. http://gerrit.cloudera.org:8080/#/c/12968/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12968/2//COMMIT_MSG@9 PS2, Line 9: and nit: an ? http://gerrit.cloudera.org:8080/#/c/12968/2//COMMIT_MSG@11 PS2, Line 11: cascade : of blocked threads Makes me wonder if there are actually other similar problematic sequence in the HdfsScanNode code ?! http://gerrit.cloudera.org:8080/#/c/12968/2/be/src/exec/hdfs-scan-node.h File be/src/exec/hdfs-scan-node.h: http://gerrit.cloudera.org:8080/#/c/12968/2/be/src/exec/hdfs-scan-node.h@117 PS2, Line 117: /// together, this lock must be taken first. Probably worth documenting briefly why this is a "timed_mutex" instead of a plain "mutex". http://gerrit.cloudera.org:8080/#/c/12968/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: http://gerrit.cloudera.org:8080/#/c/12968/2/be/src/exec/hdfs-scan-node.cc@292 PS2, Line 292: The lock can be held for significant periods of time if the scan : // node is being cancelled. Would it be clearer to say another thread could be holding this lock for an extended period of time. When I first read it, I kind of interpret it as this lock is held by _this_ function for a long time ? Also, may also help to call out that this function may be called under some other locks so that's why the remedies are being done here to break out of the loop earlier if the scan node is being cancelled. -- To view, visit http://gerrit.cloudera.org:8080/12968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4881a3e5bfda64e8d60af95ad13b450cf7f8c130 Gerrit-Change-Number: 12968 Gerrit-PatchSet: 2 Gerrit-Owner: Joe McDonnell <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 10 Apr 2019 22:25:04 +0000 Gerrit-HasComments: Yes
