Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/11228 )
Change subject: IMPALA-7450. Set thread name during refresh/load operations ...................................................................... Patch Set 1: (5 comments) This seems lovely to me and a good idea. I'm happy with it as is, except that it seems like a simple test of the annotator is plausible. I think we log thread names when we log from Java. Does that look reasonable to you? http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11228/1//COMMIT_MSG@25 PS1, Line 25: This patch is tricky to automate tests for, but I verified it manually What do you think of the following test: countdownLatchThingyInitializedToOne annotatedThread = { try(annotater) { grap semaphore; } }.run() checkusingJmxJstack that thread is annotated release the semaphore. To make it work you might need a second semaphore to lockstep the threads, but you get the idea. I'm open to this being too annoying, but it seems quick enough. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@1005 PS1, Line 1005: String annotation = String.format("invalidating metadata - %s/%s dbs complete", In an ideal world, the RPC from the impalad would be carrying a thread id, and this would have a thread id in it as well. Obviously not an actionable comment right now, but since you're knee-deep in that path, I wanted to mention it. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java File fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java: http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@26 PS1, Line 26: * that is blocked or which service is being waited upon. It's obvious, but perhaps mention that this isn't thread-safe. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@48 PS1, Line 48: public void close() { Do we want this to also double as something that logs long requests? i.e., should we log if things take more than 1 second? http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@51 PS1, Line 51: if (Thread.currentThread() == thr_ && nit: I think this could fit on one line? -- To view, visit http://gerrit.cloudera.org:8080/11228 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7c850d6bb2eedc375ee567c19eb17add335f60c Gerrit-Change-Number: 11228 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Comment-Date: Wed, 15 Aug 2018 16:19:15 +0000 Gerrit-HasComments: Yes
