Joe McDonnell 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: (4 comments) 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 ? Fixed 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 I'm thinking about whether there are things to do to detect these types of problems. The problem with this case is that it really required multiple different conditions (slow IO, lots of cancels) to hit it. 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 Added a couple sentences about why it is a timed_mutex and where we use a timeout. 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 Good point, fixed up the comment to clarify that this would be blocked on another thread and that it matters because we can hold locks that block other threads. -- 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: Thu, 11 Apr 2019 23:56:54 +0000 Gerrit-HasComments: Yes
