You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2019/07/25 22:38:43 UTC
[Impala-ASF-CR] 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()
sharanitha.harish@cloudera.com has uploaded this change for review. ( 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()
......................................................................
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() and testEventProcessorWhenNotActive() to check
1)If the metric updated as it should when new events were processed.
2)If the metric was not set when the event processor was not active.
Change-Id: I75b966f2b4eaafbcf7d80358f53501bb7ade67e7
---
M be/src/util/event-metrics.cc
M be/src/util/event-metrics.h
M common/thrift/JniCatalog.thrift
M common/thrift/metrics.json
M fe/src/main/java/org/apache/impala/catalog/events/MetastoreEventsProcessor.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
6 files changed, 37 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/25/13925/1
--
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: newchange
Gerrit-Change-Id: I75b966f2b4eaafbcf7d80358f53501bb7ade67e7
Gerrit-Change-Number: 13925
Gerrit-PatchSet: 1
Gerrit-Owner: Anonymous Coward <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] 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()
Posted by "Sharanitha Harish (Code Review)" <ge...@cloudera.org>.
Sharanitha Harish has abandoned 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()
......................................................................
Abandoned
--
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: abandon
Gerrit-Change-Id: I75b966f2b4eaafbcf7d80358f53501bb7ade67e7
Gerrit-Change-Number: 13925
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] 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()
Posted by "Sharanitha Harish (Code Review)" <ge...@cloudera.org>.
Sharanitha Harish has restored 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()
......................................................................
Restored
restoring the change
--
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: restore
Gerrit-Change-Id: I75b966f2b4eaafbcf7d80358f53501bb7ade67e7
Gerrit-Change-Number: 13925
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
[Impala-ASF-CR] 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()
Posted by "Vihang Karajgaonkar (Code Review)" <ge...@cloudera.org>.
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 <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Jul 2019 23:06:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] 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()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4008/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 23:20:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] 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()
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins 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:
(1 comment)
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: createTable(TEST_DB_NAME, "tbl_shouldtestEventProcessorMetr_skipped", tblParams, true);
line too long (91 > 90)
--
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 <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Jul 2019 22:39:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] 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()
Posted by "Sharanitha Harish (Code Review)" <ge...@cloudera.org>.
Sharanitha Harish has abandoned 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()
......................................................................
Abandoned
abandoning this commit
--
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: abandon
Gerrit-Change-Id: I75b966f2b4eaafbcf7d80358f53501bb7ade67e7
Gerrit-Change-Number: 13925
Gerrit-PatchSet: 1
Gerrit-Owner: Sharanitha Harish <sh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Vihang Karajgaonkar <vi...@cloudera.com>