hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "jiraposter@reviews.apache.org (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-3777) Redefine Identity Of HBase Configuration
Date Mon, 25 Apr 2011 21:29:03 GMT

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

jiraposter@reviews.apache.org commented on HBASE-3777:
------------------------------------------------------



bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 116
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16908#file16908line116>
bq.  >
bq.  >     Copy/paste issue (minor)

Will change it to "Default limit on concurrent client-side zookeeper connections".


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/HConstants.java, line 442
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16908#file16908line442>
bq.  >
bq.  >     Thanks for moving these configs. in here.

Yeah, the HConnectionKey would not have looked pretty if we hadn't moves those configs to
HConstants. 

I will remove the trailing space. 


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java, line 177
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16909#file16909line177>
bq.  >
bq.  >     This looks like a good change.

As a matter of fact, the CatalogTracker was the only class that was being handed a connection,
which made cleanup tricky since it didn't really own that connection (as Todd rightly pointed
out). Making it take a configuration seemed like the most pragmatic thing to do.


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java, line 63
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16910#file16910line63>
bq.  >
bq.  >     Implement Closeable now you've added close?

Yes, we can. I'll make HConnection implement Closeable as well. If you want, we can make HTablePool
implement Closeable by calling closeTablePool on all of its tables.


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 265
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16911#file16911line265>
bq.  >
bq.  >     This is painful, but makes sense.

A small price to pay, in my opinion.


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1207
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16911#file16911line1207>
bq.  >
bq.  >     Not important but if closed, just return immediately and then you can save indenting
whole method.  Not important.  Just style diff.

Will do.


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java, line 1667
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16911#file16911line1667>
bq.  >
bq.  >     So, this is just insurance as you say in the issue.  Thats fine I'd say (I agree
w/ Ted that we shouldn't rely on finalize)

Exactly - it's just insurance, a fall-back in case some thread somewhere was unable to close
the connection for whatever reason.


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/master/HMaster.java, line 355
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16917#file16917line355>
bq.  >
bq.  >     Interesting but I go along w/ it.  Looks like we only made this connection for
CT?  If so, bad design fixed by your CT change.

Yes, for the most part, the connection that was being given to CT was not used for anything
else. There was one exception though (TestCatalogTracker), which was doing all kinds of things
on the connection outside of the CT, and to accomodate that, I left open a package-level constructor
in CT that is visible only by that test case (it'd be too much trouble to change it).


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HTablePool.java, line 150
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16913#file16913line150>
bq.  >
bq.  >     Just remove.

Ok, will remove all dead code.


bq.  On 2011-04-25 20:05:54, Michael Stack wrote:
bq.  > src/main/java/org/apache/hadoop/hbase/client/HTable.java, line 259
bq.  > <https://reviews.apache.org/r/643/diff/3/?file=16912#file16912line259>
bq.  >
bq.  >     Yeah, this is ugly.... its almost as though you should have a special method
for it, one that does not up the counters?

Just a thought - how about if we hide the ugliness in HCM, like so:

  public abstract class Connectable<T> {
    public Configuration conf;

    public Connectable(Configuration conf) {
      this.conf = conf;
    }

    public abstract T connect(Connection connection);
  }

  public static <T> T execute(Connectable<T> connectable) {
    if (connectable == null || connectable.conf == null) {
      return null;
    }
    HConfiguration conf = connectable.conf;
    HConnection connection = HConnectionManager.getConnection(conf);
    try {
      return connectable.connect(connection);
    } finally {
      HConnectionManager.deleteConnection(conf, false);
    }
  }

That way, the HTable call would look somewhat prettier:

  HConnectionManager.execute(new Connectable<Boolean>(conf) {
    public Boolean connect(Connection connection) {
      return connection.isTableEnabled(tableName);
    }
  });


- Karthick


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/643/#review543
-----------------------------------------------------------


On 2011-04-22 21:16:59, Karthick Sankarachary wrote:
bq.  
bq.  -----------------------------------------------------------
bq.  This is an automatically generated e-mail. To reply, visit:
bq.  https://reviews.apache.org/r/643/
bq.  -----------------------------------------------------------
bq.  
bq.  (Updated 2011-04-22 21:16:59)
bq.  
bq.  
bq.  Review request for hbase and Ted Yu.
bq.  
bq.  
bq.  Summary
bq.  -------
bq.  
bq.  Judging from the javadoc in HConnectionManager, sharing connections across multiple clients
going to the same cluster is supposedly a good thing. However, the fact that there is a one-to-one
mapping between a configuration and connection instance, kind of works against that goal.
Specifically, when you create HTable instances using a given Configuration instance and a
copy thereof, we end up with two distinct HConnection instances under the covers. Is this
really expected behavior, especially given that the configuration instance gets cloned a lot?
bq.  
bq.  Here, I'd like to play devil's advocate and propose that we "deep-compare" HBaseConfiguration
instances, so that multiple HBaseConfiguration instances that have the same properties map
to the same HConnection instance. In case one is "concerned that a single HConnection is insufficient
for sharing amongst clients", to quote the javadoc, then one should be able to mark a given
HBaseConfiguration instance as being "uniquely identifiable".
bq.  
bq.  
bq.  This addresses bug HBASE-3777.
bq.      https://issues.apache.org/jira/browse/HBASE-3777
bq.  
bq.  
bq.  Diffs
bq.  -----
bq.  
bq.    src/main/java/org/apache/hadoop/hbase/HConstants.java 5701639 
bq.    src/main/java/org/apache/hadoop/hbase/catalog/CatalogTracker.java be31179 
bq.    src/main/java/org/apache/hadoop/hbase/client/HBaseAdmin.java afb666a 
bq.    src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java c348f7a 
bq.    src/main/java/org/apache/hadoop/hbase/client/HTable.java edacf56 
bq.    src/main/java/org/apache/hadoop/hbase/client/HTablePool.java 88827a8 
bq.    src/main/java/org/apache/hadoop/hbase/client/MetaScanner.java 9e3f4d1 
bq.    src/main/java/org/apache/hadoop/hbase/client/replication/ReplicationAdmin.java d76e333

bq.    src/main/java/org/apache/hadoop/hbase/mapreduce/replication/VerifyReplication.java
ed88bfa 
bq.    src/main/java/org/apache/hadoop/hbase/master/HMaster.java 79a48ba 
bq.    src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java d0a1e11 
bq.    src/main/java/org/apache/hadoop/hbase/replication/regionserver/ReplicationSource.java
78c3b42 
bq.    src/main/java/org/apache/hadoop/hbase/util/HBaseFsck.java 5da5e34 
bq.    src/main/java/org/apache/hadoop/hbase/util/HBaseFsckRepair.java b624d28 
bq.    src/main/java/org/apache/hadoop/hbase/zookeeper/ZKUtil.java 7f5b377 
bq.    src/main/java/org/apache/hadoop/hbase/zookeeper/ZooKeeperWatcher.java dc471c4 
bq.    src/test/java/org/apache/hadoop/hbase/catalog/TestCatalogTracker.java e25184e 
bq.    src/test/java/org/apache/hadoop/hbase/catalog/TestMetaReaderEditor.java 60320a3 
bq.    src/test/java/org/apache/hadoop/hbase/client/TestHCM.java b01a2d2 
bq.    src/test/java/org/apache/hadoop/hbase/mapreduce/TestTableMapReduce.java 624f4a8 
bq.    src/test/java/org/apache/hadoop/hbase/util/TestMergeTable.java 8992dbb 
bq.  
bq.  Diff: https://reviews.apache.org/r/643/diff
bq.  
bq.  
bq.  Testing
bq.  -------
bq.  
bq.  mvn test
bq.  
bq.  
bq.  Thanks,
bq.  
bq.  Karthick
bq.  
bq.



> Redefine Identity Of HBase Configuration
> ----------------------------------------
>
>                 Key: HBASE-3777
>                 URL: https://issues.apache.org/jira/browse/HBASE-3777
>             Project: HBase
>          Issue Type: Improvement
>          Components: client, ipc
>    Affects Versions: 0.90.2
>            Reporter: Karthick Sankarachary
>            Assignee: Karthick Sankarachary
>            Priority: Minor
>             Fix For: 0.92.0
>
>         Attachments: 3777-TOF.patch, HBASE-3777-V2.patch, HBASE-3777-V3.patch, HBASE-3777-V4.patch,
HBASE-3777-V6.patch, HBASE-3777.patch
>
>
> Judging from the javadoc in {{HConnectionManager}}, sharing connections across multiple
clients going to the same cluster is supposedly a good thing. However, the fact that there
is a one-to-one mapping between a configuration and connection instance, kind of works against
that goal. Specifically, when you create {{HTable}} instances using a given {{Configuration}}
instance and a copy thereof, we end up with two distinct {{HConnection}} instances under the
covers. Is this really expected behavior, especially given that the configuration instance
gets cloned a lot?
> Here, I'd like to play devil's advocate and propose that we "deep-compare" {{HBaseConfiguration}}
instances, so that multiple {{HBaseConfiguration}} instances that have the same properties
map to the same {{HConnection}} instance. In case one is "concerned that a single {{HConnection}}
is insufficient for sharing amongst clients",  to quote the javadoc, then one should be able
to mark a given {{HBaseConfiguration}} instance as being "uniquely identifiable".
> Note that "sharing connections makes clean up of {{HConnection}} instances a little awkward",
unless of course, you apply the change described in HBASE-3766.

--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira

Mime
View raw message