Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20507 )
Change subject: IMPALA-12460: Add lag and histogram of event processing in the log ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/20507/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20507/5//COMMIT_MSG@12 PS5, Line 12: decide Nit: line too long. http://gerrit.cloudera.org:8080/#/c/20507/5//COMMIT_MSG@12 PS5, Line 12: top-10 targets contribute Nit: _the_ top-10 targets _that_ contribute http://gerrit.cloudera.org:8080/#/c/20507/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java: http://gerrit.cloudera.org:8080/#/c/20507/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEvents.java@503 PS5, Line 503: if (tblName_ == null) return dbName_; Could add a Precondition check that 'dbName_' is not null in this case. http://gerrit.cloudera.org:8080/#/c/20507/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java File fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java: http://gerrit.cloudera.org:8080/#/c/20507/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@948 PS5, Line 948: {} We could indicate in the log that it's in milliseconds. http://gerrit.cloudera.org:8080/#/c/20507/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1106 PS5, Line 1106: // The list is sorted in ascending order. Get the item from the tail. Wouldn't it be more convenient to order it in descending order, for example using Map.Entry.comparingByValue().reversed()? In addition, we could use a range based for loop to iterate over eventList.subList(0, num). http://gerrit.cloudera.org:8080/#/c/20507/5/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1128 PS5, Line 1128: Map.Entry<String, Long> entry = targetList.get(targetList.size() - i); See L1106. -- To view, visit http://gerrit.cloudera.org:8080/20507 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9421b5e26bfa2324217ec9695fbd81636727d22 Gerrit-Change-Number: 20507 Gerrit-PatchSet: 5 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Wed, 27 Sep 2023 12:26:28 +0000 Gerrit-HasComments: Yes
