Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13925 )
Change subject: This commit adds support to display the metric last-synced-event-id as Catalogd/metrics#evnts page whereas previously it was displayed only on the catalod/events page. Added code to the MetaStoreEventsProcessorTest class under testEventProcessorMetrics() ...................................................................... Patch Set 1: (12 comments) http://gerrit.cloudera.org:8080/#/c/13925/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13925/1//COMMIT_MSG@7 PS1, Line 7: This Add a one line commit msg with the JIRA number here followed by the description text. See git log for examples. http://gerrit.cloudera.org:8080/#/c/13925/1//COMMIT_MSG@8 PS1, Line 8: evnts spell-check http://gerrit.cloudera.org:8080/#/c/13925/1//COMMIT_MSG@10 PS1, Line 10: testEventProcessorWhenNotActive nit, wrap the line around to the suggested line-width http://gerrit.cloudera.org:8080/#/c/13925/1/be/src/util/event-metrics.h File be/src/util/event-metrics.h: http://gerrit.cloudera.org:8080/#/c/13925/1/be/src/util/event-metrics.h@58 PS1, Line 58: LAST_SYNCED_EVENT_ID add a comment above this explain what it this field represents similar to the metrics above. http://gerrit.cloudera.org:8080/#/c/13925/1/be/src/util/event-metrics.h@59 PS1, Line 59: : remove unnecessary blank lines http://gerrit.cloudera.org:8080/#/c/13925/1/be/src/util/event-metrics.cc File be/src/util/event-metrics.cc: http://gerrit.cloudera.org:8080/#/c/13925/1/be/src/util/event-metrics.cc@42 PS1, Line 42: nit, remove extra blank line http://gerrit.cloudera.org:8080/#/c/13925/1/be/src/util/event-metrics.cc@66 PS1, Line 66: 0) nit, add space after comma Also, this needs to be initialized to -1 since 0 is a valid event id which may represent that there are no events in the metastore when it is initialized. http://gerrit.cloudera.org:8080/#/c/13925/1/be/src/util/event-metrics.cc@94 PS1, Line 94: : } remove unnecessary blank line. http://gerrit.cloudera.org:8080/#/c/13925/1/common/thrift/JniCatalog.thrift File common/thrift/JniCatalog.thrift: http://gerrit.cloudera.org:8080/#/c/13925/1/common/thrift/JniCatalog.thrift@785 PS1, Line 785: L nit, add space similar to comments above http://gerrit.cloudera.org:8080/#/c/13925/1/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/13925/1/common/thrift/metrics.json@2426 PS1, Line 2426: Last Synced Event Id We can more information here. Something like, "Last metastore event id which was processed by Catalog server" http://gerrit.cloudera.org:8080/#/c/13925/1/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/13925/1/fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java@548 PS1, Line 548: getLastSyncedEventId This will not publish the last synced id if the events processor is not active. When the events processor is in error state, I think there is value in knowing what was the last event id it has synced up to. So I would suggest to move this line above line 525 so that it is always published. http://gerrit.cloudera.org:8080/#/c/13925/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java: http://gerrit.cloudera.org:8080/#/c/13925/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@1447 PS1, Line 1447: tbl_shouldtestEventProcessorMetr_skipped not sure why is this change necessary? -- To view, visit http://gerrit.cloudera.org:8080/13925 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75b966f2b4eaafbcf7d80358f53501bb7ade67e7 Gerrit-Change-Number: 13925 Gerrit-PatchSet: 1 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Vihang Karajgaonkar <[email protected]> Gerrit-Comment-Date: Fri, 26 Jul 2019 23:06:32 +0000 Gerrit-HasComments: Yes
