You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/09/14 00:44:59 UTC
[kudu-CR] parse metrics log: merge metrics across entities of same type
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8062
to review the following change.
Change subject: parse_metrics_log: merge metrics across entities of same type
......................................................................
parse_metrics_log: merge metrics across entities of same type
This changes the tool so that all of the tablet metrics get summed
together before reporting. Previously we just reported the info from an
arbitrary tablet (probably the last one to be listed in the log).
This also adds a short circuit such that we skip parsing and merging of
metrics which have not been slated for output. This sped up processing
a metrics file ~10x when reporting only one or two metrics from it.
Lastly, this adds reporting of the max value from histograms.
Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
---
M src/kudu/scripts/parse_metrics_log.py
1 file changed, 80 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8062/1
--
To view, visit http://gerrit.cloudera.org:8080/8062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] parse metrics log: merge metrics across entities of same type
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: parse_metrics_log: merge metrics across entities of same type
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] parse metrics log: merge metrics across entities of same type
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: parse_metrics_log: merge metrics across entities of same type
......................................................................
Patch Set 1:
(4 comments)
First time looking at this so mostly a scan, but since it's a standalone script that probably doesn't matter so much.
http://gerrit.cloudera.org:8080/#/c/8062/1/src/kudu/scripts/parse_metrics_log.py
File src/kudu/scripts/parse_metrics_log.py:
PS1, Line 99: if k == 'name':
: continue
: if k == 'values' or k == 'counts' or k == 'min' or k == 'max' or k == 'mean':
: continue
Nit: combine?
Line 103: else:
Nit: no need for the else.
Line 161: cur.get('counts', []))
Nit: indentation here.
Line 237: if prev_data and ts < prev_data['ts'] + 30:
Worth a comment explaining this check.
--
To view, visit http://gerrit.cloudera.org:8080/8062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] parse metrics log: merge metrics across entities of same type
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: parse_metrics_log: merge metrics across entities of same type
......................................................................
parse_metrics_log: merge metrics across entities of same type
This changes the tool so that all of the tablet metrics get summed
together before reporting. Previously we just reported the info from an
arbitrary tablet (probably the last one to be listed in the log).
This also adds a short circuit such that we skip parsing and merging of
metrics which have not been slated for output. This sped up processing
a metrics file ~10x when reporting only one or two metrics from it.
Lastly, this adds reporting of the max value from histograms.
Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
Reviewed-on: http://gerrit.cloudera.org:8080/8062
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/scripts/parse_metrics_log.py
1 file changed, 91 insertions(+), 20 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/8062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] parse metrics log: merge metrics across entities of same type
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8062
to look at the new patch set (#2).
Change subject: parse_metrics_log: merge metrics across entities of same type
......................................................................
parse_metrics_log: merge metrics across entities of same type
This changes the tool so that all of the tablet metrics get summed
together before reporting. Previously we just reported the info from an
arbitrary tablet (probably the last one to be listed in the log).
This also adds a short circuit such that we skip parsing and merging of
metrics which have not been slated for output. This sped up processing
a metrics file ~10x when reporting only one or two metrics from it.
Lastly, this adds reporting of the max value from histograms.
Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
---
M src/kudu/scripts/parse_metrics_log.py
1 file changed, 91 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/8062/2
--
To view, visit http://gerrit.cloudera.org:8080/8062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] parse metrics log: merge metrics across entities of same type
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: parse_metrics_log: merge metrics across entities of same type
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8062/1/src/kudu/scripts/parse_metrics_log.py
File src/kudu/scripts/parse_metrics_log.py:
PS1, Line 99: if k == 'name':
: continue
: if k == 'values' or k == 'counts' or k == 'min' or k == 'max' or k == 'mean':
: continue
> Nit: combine?
Done
Line 103: else:
> Nit: no need for the else.
Done
Line 161: cur.get('counts', []))
> Nit: indentation here.
Done
Line 237: if prev_data and ts < prev_data['ts'] + 30:
> Worth a comment explaining this check.
moved the magic value 30 to a constant. Also added an extra check here to make sure that timestamps are ascending (an assumption of the code but not enforced prior)
--
To view, visit http://gerrit.cloudera.org:8080/8062
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I442cddd4c5352dcfae30fa23888082d916069d5f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes