Details

The misleading "getRequestLatency", "deleteRequestLatency", and "putRequestLatency" histograms were removed from metrics because such requests often were not actually measured.

Description

A client application (LoadTestTool) calls put() on HTables. Internally to the HBase client those puts are batched into MultiActions. The total number of put operations shown in the RegionServer's put metrics histogram never increases from 0 even though millions of such operations are made. Needless to say the latency for those operations are not measured either. The value of HBASE-5533 metrics are suspect given the client will batch put and delete ops like this.

I had a fix in progress but HBASE-6284 messed it up. Before, MultiAction processing in HRegionServer would distingush between puts and deletes and dispatch them separately. It was easy to account for the time for them. Now both puts and deletes are submitted in batch together as mutations.

Andrew Purtell
added a comment - 12/Jul/12 02:28 Other means for applying puts and deletes (RowMutations etc.) are not covered either, neither are Increments, or Appends. Perhaps this is an argument for reverting HBASE-5533 .

Lars Hofhansl
added a comment - 12/Jul/12 03:52 I "knew" there would be issues somewhere with HBASE-6284
What do you say Andrew, this seems bad enough to delay 0.94.1?
Does a "Put" metric still make sense? Should it be a "Mutation" metric which includes Deletes?

Perhaps not a full revert, but the "put" and "delete" metrics are not useful in a basic LoadTestTool test scenario, so consider dropping those. The distinction is increasingly only valid client side. Perhaps also remove the "get" one as well so we're not in effect special casing a metric only into HRI.get().

Edit: But even after the above, the histograms remain for FS level ops, so there's benefit to a partial revert only.

Andrew Purtell
added a comment - 12/Jul/12 05:06 - edited Perhaps not a full revert, but the "put" and "delete" metrics are not useful in a basic LoadTestTool test scenario, so consider dropping those. The distinction is increasingly only valid client side. Perhaps also remove the "get" one as well so we're not in effect special casing a metric only into HRI.get().
Edit: But even after the above, the histograms remain for FS level ops, so there's benefit to a partial revert only.

Perhaps we can have "Get" and "Update" metrics. "Updates" would include Put, Deleted, ICV, etc.
But maybe that would require more discussion, so short term (0.94.1 at least), we could remove the Put/Delete/Get metrics as you suggest.

Lars Hofhansl
added a comment - 12/Jul/12 05:57 Perhaps we can have "Get" and "Update" metrics. "Updates" would include Put, Deleted, ICV, etc.
But maybe that would require more discussion, so short term (0.94.1 at least), we could remove the Put/Delete/Get metrics as you suggest.

On trunk apparently all use of the "get", "put", and "delete" histograms have already been removed from the regionserver, except for the Jamon templates. Since an approach to put back two such HRegionInterface level measurements with a query/mutate distinction has been +1ed on this issue I'm going to put them back on trunk and port back the changes to 0.94. Calling them queryRequestLatency and mutateRequestLatency in RegionServerMetrics.

Andrew Purtell
added a comment - 12/Jul/12 17:59 On trunk apparently all use of the "get", "put", and "delete" histograms have already been removed from the regionserver, except for the Jamon templates. Since an approach to put back two such HRegionInterface level measurements with a query/mutate distinction has been +1ed on this issue I'm going to put them back on trunk and port back the changes to 0.94. Calling them queryRequestLatency and mutateRequestLatency in RegionServerMetrics.

Attached as '6377-trunk-simple.patch' for your consideration is a minimal set of changes that put back HRegionInterface level request metrics. The intent of HBASE-5533 was to have server side request metrics to match up with client side metrics. So what we are measuring here? Here is my interpretation: Given all of the batching/dispatch/increasingly async actions taken by the client a 1:1 correspondence with client API ops is not what we are after; instead, a measure of the server side time required to process a unit of work sent by the client. This kind of measurement can be done simply and cheaply, especially given the nice HRI refactor in trunk. There's no need to ask "how to deconstruct this server side packet of work into client side ops and account for them individually?" On the other hand, it's a very coarse measurement.

Andrew Purtell
added a comment - 12/Jul/12 18:44 Attached as '6377-trunk-simple.patch' for your consideration is a minimal set of changes that put back HRegionInterface level request metrics. The intent of HBASE-5533 was to have server side request metrics to match up with client side metrics. So what we are measuring here? Here is my interpretation: Given all of the batching/dispatch/increasingly async actions taken by the client a 1:1 correspondence with client API ops is not what we are after; instead, a measure of the server side time required to process a unit of work sent by the client. This kind of measurement can be done simply and cheaply, especially given the nice HRI refactor in trunk. There's no need to ask "how to deconstruct this server side packet of work into client side ops and account for them individually?" On the other hand, it's a very coarse measurement.

Looks good to me. Reasoning makes sense.
We can report per op latency just by dividing the batch latency by the batch size (still no need to deconstruct per individual op). Not sure if that more useful or less.

Does this need more discussion? If so, maybe the best course of action is to revert HBASE-5533 now and regroup.

Lars Hofhansl
added a comment - 12/Jul/12 19:29 - edited Looks good to me. Reasoning makes sense.
We can report per op latency just by dividing the batch latency by the batch size (still no need to deconstruct per individual op). Not sure if that more useful or less.
Does this need more discussion? If so, maybe the best course of action is to revert HBASE-5533 now and regroup.

Andrew Purtell
added a comment - 12/Jul/12 20:28 We can report per op latency just by dividing the batch latency by the batch size
Doing that. But, as before, for N ops you have to submit (totalTime/N) N times to the histogram.

stack
added a comment - 12/Jul/12 21:07 Doing that. But, as before, for N ops you have to submit (totalTime/N) N times to the histogram.
That seems wonky Mr Purtell?
Patch seems fine to me.
Mr Elliott, you have carnal knowledge of metrics, any opinion?

Andrew Purtell
added a comment - 12/Jul/12 21:11 Attached are candidates for trunk and 0.94 currently under test. The 0.94 version is noisier than I'd prefer but it fills in where we were missing coverage before.

Tangentially, if we're doing all over the place a lot of this get nanos then get them again and subtract the diff, we might want to just use Guava's Stopwatch. Also, in our internal version of 0.94 we've scaled metrics to milliseconds. The Stopwatch javadocs talk about that: "Note that the overhead of measurement can be more than a microsecond, so it is generally not useful to specify TimeUnit.NANOSECONDS precision here." For another JIRA perhaps.

Andrew Purtell
added a comment - 12/Jul/12 21:18 Tangentially, if we're doing all over the place a lot of this get nanos then get them again and subtract the diff, we might want to just use Guava's Stopwatch. Also, in our internal version of 0.94 we've scaled metrics to milliseconds. The Stopwatch javadocs talk about that: "Note that the overhead of measurement can be more than a microsecond, so it is generally not useful to specify TimeUnit.NANOSECONDS precision here." For another JIRA perhaps.

The trunk patch looks great.
The 0.94 patch, Will give some really weird numbers I think. The histogram could be dominated by the bulk insert operation. Should that be switched to just a PersistantTimeVaryingRate? Better to give less information, but for it to be accurate.

Elliott Clark
added a comment - 12/Jul/12 22:07 The trunk patch looks great.
The 0.94 patch, Will give some really weird numbers I think. The histogram could be dominated by the bulk insert operation. Should that be switched to just a PersistantTimeVaryingRate? Better to give less information, but for it to be accurate.

Please correct my understanding on this if I'm wrong, but estimating via the average (total latency / num ops) is pretty undesirable for both normal MultiActions and the batched put MultiAction case. Latency from a slow op bleeds over to a fast one, which messes up per-op metrics. This would also affect doing per-region or per-column family metrics in the future, for the same reasons.

I haven't looked at the code, but is it possible to do more accurate accounting of the latency of each op in a MultiAction? If so, I think it'd be worthwhile.

Andrew Wang
added a comment - 12/Jul/12 22:21 Please correct my understanding on this if I'm wrong, but estimating via the average (total latency / num ops) is pretty undesirable for both normal MultiActions and the batched put MultiAction case. Latency from a slow op bleeds over to a fast one, which messes up per-op metrics. This would also affect doing per-region or per-column family metrics in the future, for the same reasons.
I haven't looked at the code, but is it possible to do more accurate accounting of the latency of each op in a MultiAction? If so, I think it'd be worthwhile.

If you mean Put(List) or MultiAction, each dispatch down to HRegion is individually accounted. A big limitation we have with this approach of measuring at HRI is we can't drop down into the region to account for each op individually. Current 0.94 patch maintains how the HBASE-5533 patch did it.

I believe such issues of larger rethinking were what Lars had in mind in comments above. TBD.

Andrew Purtell
added a comment - 12/Jul/12 22:23 The histogram could be dominated by the bulk insert operation.
If you mean Put(List) or MultiAction, each dispatch down to HRegion is individually accounted. A big limitation we have with this approach of measuring at HRI is we can't drop down into the region to account for each op individually. Current 0.94 patch maintains how the HBASE-5533 patch did it.
I believe such issues of larger rethinking were what Lars had in mind in comments above. TBD.

Andrew Purtell
added a comment - 13/Jul/12 00:33 - edited @Elliot we should do the same on trunk and fix up the UI.
Edit: Currently the only place those histograms are referenced are the Jamon templates.

Elliott Clark
added a comment - 13/Jul/12 00:46 @Andrew
They're exposed through jmx too. All the more reason to get them correct if we're going to do them.
+1 on trunk patch. After 4050 goes in I'm going to try and get to some of the metric re-writes that are needed. Looks like this will be one on the list.