Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20473 )
Change subject: IMPALA-11553: Add event specific metrics in the table metrics ...................................................................... Patch Set 2: (4 comments) Thanks for adding these! I just take a quick look. Will look into this deeper. http://gerrit.cloudera.org:8080/#/c/20473/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20473/2//COMMIT_MSG@22 PS2, Line 22: Can we add more metrics at table level, e.g. events-skipped, last-synced-event-id, events-consuming-delay-ms? http://gerrit.cloudera.org:8080/#/c/20473/2/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/20473/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@942 PS2, Line 942: latestEventTimeMs_.set(event.getEventTime()); I made a mistake that the unit is actually second, not milli second. Could you also correct the variable name and the metric text appropriately? E.g. latestEventTimeMs_ -> latestEventTimeSecs_ lastSyncedEventTimeMs_ -> lastSyncedEventTimeSecs_ "latest-event-time-ms" -> "latest-event-time-secs" "last-synced-event-time-ms" -> "last-synced-event-time-secs" "event-processing-delay-ms" -> "event-processing-delay-secs" http://gerrit.cloudera.org:8080/#/c/20473/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1063 PS2, Line 1063: MILLISECONDS TimeUnit.SECONDS ? http://gerrit.cloudera.org:8080/#/c/20473/2/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@1078 PS2, Line 1078: public long now() { We use 'System.currentTimeMillis() / 1000' directly in many places. Maybe don't worth a method for this, and it's ambigous what the unit is. $ git grep 'System.currentTimeMillis() / 1000' fe/src/main/java/org/apache/impala/catalog/Table.java: msTbl.putToParameters(propertyKey, Long.toString(System.currentTimeMillis() / 1000)); fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java: (int) (System.currentTimeMillis() / 1000), fe/src/main/java/org/apache/impala/hive/executor/HiveJavaFunction.java: (int) (System.currentTimeMillis() / 1000), FunctionType.JAVA, resources); fe/src/test/java/org/apache/impala/service/JdbcTest.java: lastTimeSessionActive.add(System.currentTimeMillis() / 1000); fe/src/test/java/org/apache/impala/service/JdbcTest.java: long now = System.currentTimeMillis() / 1000; -- To view, visit http://gerrit.cloudera.org:8080/20473 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2428029361e610a0fcd8ed11be2ab771f03b00dd Gerrit-Change-Number: 20473 Gerrit-PatchSet: 2 Gerrit-Owner: Sai Hemanth Gantasala <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Tue, 12 Sep 2023 00:42:05 +0000 Gerrit-HasComments: Yes
