You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Vincent Tran (Code Review)" <ge...@cloudera.org> on 2018/09/04 15:43:02 UTC
[Impala-ASF-CR] IMPALA-6758: Add metric for current catalog version to catalog
Vincent Tran has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11383
Change subject: IMPALA-6758: Add metric for current catalog version to catalog
......................................................................
IMPALA-6758: Add metric for current catalog version to catalog
This change adds a new metric to the impalad web UI under
the MetricGroup catalog:
- catalog.age
It tracks the elapsed time since the last consumed catalog
topic update was generated.
Testing:
* Manually verified that the metric displays in the impalad
web UI and that it displays the expected age of the consumed
catalog topic update.
* Added a web server e2e test to verify that the impalad web UI
returns a /metrics page containing the new metric name.
Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/CatalogObjects.thrift
M common/thrift/Frontend.thrift
M common/thrift/metrics.json
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M tests/webserver/test_web_pages.py
10 files changed, 50 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11383/1
--
To view, visit http://gerrit.cloudera.org:8080/11383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
Gerrit-Change-Number: 11383
Gerrit-PatchSet: 1
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
[Impala-ASF-CR] IMPALA-6758: Add metric for current catalog version to catalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11383 )
Change subject: IMPALA-6758: Add metric for current catalog version to catalog
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11383/1/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11383/1/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@218
PS1, Line 218: CatalogObjectVersionSet.INSTANCE.getMinimumVersion(), newCatalogVersion);
tab used for whitespace
--
To view, visit http://gerrit.cloudera.org:8080/11383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
Gerrit-Change-Number: 11383
Gerrit-PatchSet: 1
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 04 Sep 2018 16:07:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6758: Add metric for current catalog version to catalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11383 )
Change subject: IMPALA-6758: Add metric for current catalog version to catalog
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/571/ : 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/11383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
Gerrit-Change-Number: 11383
Gerrit-PatchSet: 2
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 04 Sep 2018 16:26:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6758: Add metric for current catalog version to catalog
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/11383 )
Change subject: IMPALA-6758: Add metric for current catalog version to catalog
......................................................................
Patch Set 2:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/11383/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11383/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-6758: Add metric for current catalog version to catalog
nit: update the message appropriately
http://gerrit.cloudera.org:8080/#/c/11383/2//COMMIT_MSG@11
PS2, Line 11: age
I think maybe 'lag' would be better here, since I would consider the "age" of the catalog to be something like the uptime
http://gerrit.cloudera.org:8080/#/c/11383/2/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/11383/2/be/src/service/impala-server.h@1047
PS2, Line 1047: int64_t catalog_age;
I think the variable name should include the units (is this seconds, millis, micros, etc). Perhaps the metric itself should also include units?
http://gerrit.cloudera.org:8080/#/c/11383/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/11383/2/be/src/service/impala-server.cc@1463
PS2, Line 1463: ImpaladMetrics::CATALOG_AGE->SetValue(catalog_age);
we should think about how we want to handle negative age -- it's possible if the clocks are slightly out of sync. Maybe we should clamp to zero? Or we should document in the description of the metric that this metric's accuracy relies on wall clock synchronization, and a negative value indicates some potential for lag?
http://gerrit.cloudera.org:8080/#/c/11383/2/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:
http://gerrit.cloudera.org:8080/#/c/11383/2/common/thrift/CatalogObjects.thrift@593
PS2, Line 593: // The genesis time of the catalog object.
The comment and the name of the field should be clearer as to units and reference point (is this seconds since unix epoch?)
Also I think the word "genesis" might be a little too fancy, maybe it would be clearer to say something like "publish_timestamp"?
http://gerrit.cloudera.org:8080/#/c/11383/2/common/thrift/Frontend.thrift
File common/thrift/Frontend.thrift:
http://gerrit.cloudera.org:8080/#/c/11383/2/common/thrift/Frontend.thrift@717
PS2, Line 717: 4: optional i64 catalog_age
same naming concerns as elsewhere
http://gerrit.cloudera.org:8080/#/c/11383/2/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/11383/2/common/thrift/metrics.json@213
PS2, Line 213: elapsed time
should this say "in seconds" or does the units below already make it clear? I'm not sure how this JSON is consumed.
http://gerrit.cloudera.org:8080/#/c/11383/2/common/thrift/metrics.json@217
PS2, Line 217: The elapsed time since the last consumed catalog update was created
I think the label is supposed to be something shorter
http://gerrit.cloudera.org:8080/#/c/11383/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
File fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java:
http://gerrit.cloudera.org:8080/#/c/11383/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@163
PS2, Line 163: long newCatalogGenesis = lastSyncedCatalogGenesis_;
is it worth tracking this state? it seems it's only necessary to prevent backward movement, but this whole thing is just for a metric anyway, so backward movement isn't the end of the world.
http://gerrit.cloudera.org:8080/#/c/11383/2/fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java@225
PS2, Line 225: res.setCatalog_age(elapsedTime);
calculating the age here implies that it only gets re-calculated when updates are received. If you were to kill -STOP the statestore, I think you'd find that no updates are getting sent to the catalog, and then the metric would freeze instead of creep up, no? I think you need to publish back the timestamp to the backend so that it can recalculate the lag either periodically or "on demand" when the metric is fetched
http://gerrit.cloudera.org:8080/#/c/11383/2/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:
http://gerrit.cloudera.org:8080/#/c/11383/2/tests/webserver/test_web_pages.py@228
PS2, Line 228: def test_catalog_age_metric(self):
perhaps we can have a more thorough test that does a kill -STOP on the statestore, and watches to make sure the catalog age actually increases after 5-10 seconds?
--
To view, visit http://gerrit.cloudera.org:8080/11383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
Gerrit-Change-Number: 11383
Gerrit-PatchSet: 2
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 04 Sep 2018 16:37:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6758: Add metric for current catalog version to catalog
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
Hello Lars Volker, Tianyi Wang, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11383
to look at the new patch set (#2).
Change subject: IMPALA-6758: Add metric for current catalog version to catalog
......................................................................
IMPALA-6758: Add metric for current catalog version to catalog
This change adds a new metric to the impalad web UI under
the MetricGroup catalog:
- catalog.age
It tracks the elapsed time since the last consumed catalog
topic update was generated.
Testing:
* Manually verified that the metric displays in the impalad
web UI and that it displays the expected age of the consumed
catalog topic update.
* Added a web server e2e test to verify that the impalad web UI
returns a /metrics page containing the new metric name.
Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
---
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/util/impalad-metrics.cc
M be/src/util/impalad-metrics.h
M common/thrift/CatalogObjects.thrift
M common/thrift/Frontend.thrift
M common/thrift/metrics.json
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M tests/webserver/test_web_pages.py
10 files changed, 50 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/11383/2
--
To view, visit http://gerrit.cloudera.org:8080/11383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
Gerrit-Change-Number: 11383
Gerrit-PatchSet: 2
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-6758: Add metric for current catalog version to catalog
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11383 )
Change subject: IMPALA-6758: Add metric for current catalog version to catalog
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/570/ : 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/11383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
Gerrit-Change-Number: 11383
Gerrit-PatchSet: 1
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 04 Sep 2018 16:16:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6758: Add metric for current catalog version to catalog
Posted by "Vincent Tran (Code Review)" <ge...@cloudera.org>.
Vincent Tran has abandoned this change. ( http://gerrit.cloudera.org:8080/11383 )
Change subject: IMPALA-6758: Add metric for current catalog version to catalog
......................................................................
Abandoned
Abandoned for now until I have more time to reevaluate.
--
To view, visit http://gerrit.cloudera.org:8080/11383
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I82e51e8cd2f6a77c588914242684b885816aa0a0
Gerrit-Change-Number: 11383
Gerrit-PatchSet: 2
Gerrit-Owner: Vincent Tran <vt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <tw...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>