Paul Rogers 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 2: (5 comments) Applied code review comments. Added a test case. Bharath, can you give this a review and see if it is ready to go? 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: by jstacking a catalogd while performing some workload. Also added a > k, I'll add a simple test. I think just doing it single-threaded and assert Created an automated, synchronized test. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/TableLoader.java File fe/src/main/java/org/apache/impala/catalog/TableLoader.java: http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/catalog/TableLoader.java@105 PS1, Line 105: sw.elapsedTime(TimeUnit.MILLISECONDS) > Can this be rolled into ThreadNameAnnotator (or InstrumentedScope) and log Could, but requires passing the logger into the annotator so the the log message is attributed to the proper class. Also, would have to pass in the log text. Actually, seems simpler to do it as shown here: the worker thread has much more context than the annotator does. 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: * reading a jstack. For example, when making calls to external services which might > It's obvious, but perhaps mention that this isn't thread-safe. Added a bit more explanation. http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@51 PS1, Line 51: thr_.setName(newName_); > nit: I think this could fit on one line? Done http://gerrit.cloudera.org:8080/#/c/11228/1/fe/src/main/java/org/apache/impala/util/ThreadNameAnnotator.java@51 PS1, Line 51: .setName(newName_); > When is this possible? (Thread.currentThread() != thr_) Same question. Since the annotator sits in the executing thread itself (not outside), I can't think of a way that the thread can replace itself. In fact, the only way this could happen is if someone made the mistake of setting the thread name in one thread, and calling close in another. Changed to use Preconditions to check for this error case. -- 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: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 26 Jan 2019 02:10:12 +0000 Gerrit-HasComments: Yes
