hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars George (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-13270) Setter for Result#getStats is #addResults; confusing!
Date Fri, 20 Mar 2015 07:03:38 GMT

    [ https://issues.apache.org/jira/browse/HBASE-13270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14370874#comment-14370874

Lars George commented on HBASE-13270:

Since the I am not sure what the original intent was, maybe [~jesse_yates] could chime in?

>From the usage which does this

        } else if (roe.hasResult()) {
          responseValue = ProtobufUtil.toResult(roe.getResult(), cells);
          // add the load stats, if we got any
          if (roe.hasLoadStats()) {
            ((Result) responseValue).addResults(roe.getLoadStats());

it looks like a one off call, i.e. straight assignment as the code does internally already.
In this case I am +1 for naming it {{setStats()}} instead as you suggest.

In fact, I am a hater for these short names anyways (all IDEs with autocompletion take this
pain away these days) and would rather suggest we use {{setStatistics()/getStatistics()}}
- and since this is arbitrary, even better {{setRegionLoadStatistics()/getRegionLoadStatistics()}}.
This makes it much more user friendly without having to resort to JavaDoc and other measures.

> Setter for Result#getStats is #addResults; confusing!
> -----------------------------------------------------
>                 Key: HBASE-13270
>                 URL: https://issues.apache.org/jira/browse/HBASE-13270
>             Project: HBase
>          Issue Type: Improvement
>            Reporter: stack
>              Labels: beginner
>         Attachments: HBASE-13270.patch
> Below is our [~larsgeorge] on a finding he made reviewing our API:
> "Result class having getStats() and addResults(Stats) makes little sense..."
> "...the naming is just weird. You have a getStats() getter and an addResults(Stats) setter???"
> "...Especially in the Result class and addResult() is plain misleading..."
> This issue is about deprecating addResults and replacing it with addStats in its place.
> The getStats/addResult is recent. It came in with:
> {code}
> commit a411227b0ebf78b4ee8ae7179e162b54734e77de
> Author: Jesse Yates <jesse.k.yates@gmail.com>
> Date:   Tue Oct 28 16:14:16 2014 -0700
>     HBASE-5162 Basic client pushback mechanism
> ...
> {code}
> RegionLoadStats don't belong in Result if you ask me but better in the enveloping on
invocations... but that is another issue.

This message was sent by Atlassian JIRA

View raw message