Re: Forcing separate connections Was: backporting HBASE-3777 to 0.90

[Ted Yu <yu...@gmail.com>]

Gary, Todd:
Please take a look at patch v4 for HBASE-4508 and give feedback on the next
step.

Thanks

On Thu, Sep 29, 2011 at 6:52 AM, Ted Yu <yu...@gmail.com> wrote:

> Thanks for being considerate, Bright.
>
> For this new parameter, instead of hbase.client.per.config.inst, can we
> name it 'hbase.connection.per.config' ?
> Basically from its description, connection sharing is concerned.
>
> Cheers
>
>
> On Thu, Sep 29, 2011 at 6:47 AM, Bright Fulton <br...@gmail.com>wrote:
>
>> Gary, Todd, Ted, although 0.90.3 hbase-rest (CDH3u0, u1) leaks ZK
>> connections on every request (via HBaseAdmin ctor), preserving client
>> behavior in a point release is a good idea, so for discussion I've
>> included an hbase.client.per.config.inst param which defaults to true
>> in the v1 patch attached to HBASE-4508.
>>
>> Bright
>>
>>
>> On Wed, Sep 28, 2011 at 6:22 PM, Gary Helmling <gh...@gmail.com>
>> wrote:
>> > Hi Ted,
>> >
>> > Thanks for pointing it out.  Looking through the patch I did see that
>> > forcing separate connections was supported by tweaking the instance ID
>> > value.  The problem I'm pointing out is not that it can't be done, but
>> that
>> > it would require code changes on the user's part.  As an HBase user,
>> this is
>> > not what I would expect when doing a minor version upgrade.
>> >
>> > Admittedly, the scenario I'm calling out is likely to be rare (maybe no
>> one
>> > at all is doing it).  But it is valid.
>> >
>> > I don't want us to be ossified by compatibility concerns or preserve
>> poor
>> > behavior because that's the way it was previously done.  On the other
>> hand,
>> > I think we should only be making incompatible changes in a minor release
>> if
>> > there are extremely compelling reasons for doing so.  The fact that
>> there
>> > are many installs successfully running 0.90 without this patch makes me
>> > question whether or not it's extremely compelling.
>> >
>> > Are there other fixes that this patch provides to connection handling
>> > (outside of the HConnectionKey identity) that are otherwise broken in
>> 0.90
>> > (previous connection leaks, etc)?
>> >
>> > I completely agree that HBASE-3777 is a better approach to connection
>> > handling than the previous object identity behavior and I think it
>> creates a
>> > better end user experience.  But it is a disruptive change.
>> >
>> > At the very least, if this change goes in, it must be clearly flagged as
>> an
>> > incompatible change with a explanation of the changed behavior in
>> release
>> > notes.
>> >
>> > --gh
>> >
>> >
>> > On Wed, Sep 28, 2011 at 2:51 PM, Ted Yu <yu...@gmail.com> wrote:
>> >
>> >> Gary:
>> >> Karthick and I devised the following
>> (HConstants.HBASE_CLIENT_INSTANCE_ID)
>> >> for the scenario you listed below:
>> >>
>> >>  /**
>> >>   * Parameter name for unique identifier for this {@link Configuration}
>> >>   * instance. If there are two or more {@link Configuration} instances
>> >> that,
>> >>   * for all intents and purposes, are the same except for their
>> instance
>> >> ids,
>> >>   * then they will not be able to share the same {@link Connection}
>> >> instance.
>> >>   * On the other hand, even if the instance ids are the same, it could
>> >> result
>> >>   * in non-shared {@link Connection} instances if some of the other
>> >> connection
>> >>   * parameters differ.
>> >>   */
>> >>  public static String HBASE_CLIENT_INSTANCE_ID = "
>> hbase.client.instance.id
>> >> ";
>> >>
>> >> FYI
>> >>
>> >> On Wed, Sep 28, 2011 at 12:06 PM, Gary Helmling <gh...@gmail.com>
>> >> wrote:
>> >>
>> >> > Changing the connection identity behavior in the middle of a release
>> >> series
>> >> > seems like a bad idea.
>> >> >
>> >> > The 0.20 releases did connection identity based on Configuration
>> >> contents,
>> >> > 0.90 changed this to Configuration instance identity, then 0.90.5
>> would
>> >> be
>> >> > going back to contents again (acknowledged with a smarter subset and
>> >> guards
>> >> > against changes)?  If anyone running 0.90 relies on the current
>> behavior
>> >> to
>> >> > enforce separate connections (for whatever reason), using separate
>> >> > Configuration instances, this would break that behavior and appear as
>> a
>> >> > regression right?
>> >> >
>> >> > Changing these underlying assumptions in a minor release doesn't seem
>> >> > right.  I agree it's nice to have the backport for those interested
>> in
>> >> > trying it.  But I'd need some convincing that the current 0.90
>> behavior
>> >> is
>> >> > completely broken rather than sub-optimal to agree to include it.
>> >> >
>> >> > --gh
>> >> >
>> >> >
>> >> > On Wed, Sep 28, 2011 at 9:55 AM, Ted Yu <yu...@gmail.com> wrote:
>> >> >
>> >> > > One reason for my endorsement is that it would take 0.92 quite some
>> >> time
>> >> > to
>> >> > > reach the level of stability of 0.90.4
>> >> > > I really think HBASE-3777 would benefit HBase users a lot, and
>> reducing
>> >> > > potential future inquiry about connection-related issues.
>> >> > >
>> >> > > Of course, backporting increases the amount of work for validation
>> of
>> >> > > 0.90.5
>> >> > > But I think it is worth it.
>> >> > >
>> >> > > My two cents.
>> >> > >
>> >> > > On Wed, Sep 28, 2011 at 9:47 AM, Jean-Daniel Cryans <
>> >> jdcryans@apache.org
>> >> > > >wrote:
>> >> > >
>> >> > > > I'm -0 at the moment, it's a big patch to include in a point
>> release.
>> >> > > >
>> >> > > > I'm glad the work was done tho because it means those interested
>> >> (like
>> >> > > > me) can directly patch it in and test it (at my own risk).
>> >> > > >
>> >> > > > J-D
>> >> > > >
>> >> > > > On Wed, Sep 28, 2011 at 9:29 AM, Ted Yu <yu...@gmail.com>
>> wrote:
>> >> > > > > Hi,
>> >> > > > > Bright Fulton has volunteered to backport HBASE-3777 to 0.90
>> >> > > > > I endorse his effort.
>> >> > > > >
>> >> > > > > If you have comment(s), please share.
>> >> > > > >
>> >> > > > > I will open a new JIRA for this effort if this motion passes.
>> >> > > > >
>> >> > > > > Thanks
>> >> > > > >
>> >> > > >
>> >> > >
>> >> >
>> >>
>> >
>>
>
>