Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4187: Switch RPC latency metrics to histograms
......................................................................


Patch Set 3:

> (1 comment)
 > 
 > Can you give an example of what the old and new metries look like? 
 > Does CM need to be updated at all?

CM doesn't collect these metrics yet (I checked their code before), but this 
reminds me we should update common/thrift/metrics.json

The rpc-method

  {
    "description": "Duration (ms) of RPC calls to $0",
    "contexts": [
      "CATALOGSERVER",
      "STATESTORE",
      "IMPALAD"
    ],
    "label": "$0 RPC Call Duration",
    "units": "TIME_MS",
    "kind": "STATS",
    "key": "rpc-method.$0.call_duration"
  },


STATS -> HISTOGRAM

I expected there to be a runtime check to catch this mistake, but it looks like 
we check the metric "type" in SimpleMetric and not in HistogramMetric.

The histogram metric constructor should

DCHECK_EQ(TMetricKind::HISTOGRAM, metric_def.kind);

-- 
To view, visit http://gerrit.cloudera.org:8080/4516
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9ba6d4270dd5676eeeff35ad8d9dc5dcddd95e34
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Juan Yu <j...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com>
Gerrit-HasComments: No

Reply via email to