Jim Apple has posted comments on this change. ( http://gerrit.cloudera.org:8080/10145 )
Change subject: IMPALA-5392: Added all stack frames to ThreadInfo summary. ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/10145/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10145/4//COMMIT_MSG@10 PS4, Line 10: This nit: replace with ", which " to make it clear you are talking about ThreadInfo.toString, not your patch. http://gerrit.cloudera.org:8080/#/c/10145/4//COMMIT_MSG@11 PS4, Line 11: only nit: elide http://gerrit.cloudera.org:8080/#/c/10145/4//COMMIT_MSG@13 PS4, Line 13: No other detail is added or edited or omitted apart from : the constraint stated above. : : A standard ThreadInfo summary includes - : - Thread ID : - Thread Name : - Thread State : - Entire Stack Trace (if any) : - Monitor Info (if any) : - Lock Info nit: elide http://gerrit.cloudera.org:8080/#/c/10145/2/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: http://gerrit.cloudera.org:8080/#/c/10145/2/fe/src/main/java/org/apache/impala/common/JniUtil.java@262 PS2, Line 262: if (threadInfo.getLockName() != null) { > Of the many methods in https://docs.oracle.com/javase/8/docs/api/java/lang/ When a new patch set is updated, the author will typically respond to these comments. http://gerrit.cloudera.org:8080/#/c/10145/2/fe/src/main/java/org/apache/impala/common/JniUtil.java@275 PS2, Line 275: nd > This is all output only in HTML, yes, where all white space is equivalent? Why '\n' rather than ' '? http://gerrit.cloudera.org:8080/#/c/10145/2/fe/src/main/java/org/apache/impala/common/JniUtil.java@296 PS2, Line 296: for (MonitorInfo mi : threadInfo.getLockedMonitors()) { > Please add a note explaining why you fall-through here, rather than printin Missed comment. http://gerrit.cloudera.org:8080/#/c/10145/2/fe/src/main/java/org/apache/impala/common/JniUtil.java@300 PS2, Line 300: } > I'm not sure what the scale is here, but this is not a great way to check t missed comment. http://gerrit.cloudera.org:8080/#/c/10145/4/fe/src/main/java/org/apache/impala/common/JniUtil.java File fe/src/main/java/org/apache/impala/common/JniUtil.java: http://gerrit.cloudera.org:8080/#/c/10145/4/fe/src/main/java/org/apache/impala/common/JniUtil.java@24 PS4, Line 24: import java.lang.StackTraceElement; : import java.lang.Thread.State; Not alphabetical with below. http://gerrit.cloudera.org:8080/#/c/10145/4/fe/src/main/java/org/apache/impala/common/JniUtil.java@248 PS4, Line 248: nit: lines should not have trailing spaces. http://gerrit.cloudera.org:8080/#/c/10145/4/fe/src/main/java/org/apache/impala/common/JniUtil.java@249 PS4, Line 249: typically Why not include lock name and the others in this comment? -- To view, visit http://gerrit.cloudera.org:8080/10145 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I80ab4aad03e0c1f01fecad6b87779531244c28b7 Gerrit-Change-Number: 10145 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Sharma <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Charles Agnello <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Comment-Date: Wed, 25 Apr 2018 17:47:20 +0000 Gerrit-HasComments: Yes
