[
https://issues.apache.org/jira/browse/HBASE-3777?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13026061#comment-13026061
]
jirapos...@reviews.apache.org commented on HBASE-3777:
------------------------------------------------------
bq. On 2011-04-28 00:21:00, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java,
line 130
bq. > <https://reviews.apache.org/r/643/diff/3/?file=16911#file16911line130>
bq. >
bq. > hashcode -> TableServers is not quite right. It's not a map of
hashcode - that implies that two confs that happened to hash to the same code
would share an HConnectionImpl, which isn't true.
Changed the comment to read "A LRU Map of HConnectionKey -> HConnection
(TableServer)".
bq. On 2011-04-28 00:21:00, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java,
line 179
bq. > <https://reviews.apache.org/r/643/diff/3/?file=16911#file16911line179>
bq. >
bq. > This increment has to be inside the synchronized (HBASE_INSTANCES)
or else there's a race against deleteConnection.
I believe the increment is inside the synchronized block.
bq. On 2011-04-28 00:21:00, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java,
line 411
bq. > <https://reviews.apache.org/r/643/diff/3/?file=16911#file16911line411>
bq. >
bq. > this atomicinteger isn't ever used atomically (it's always under a
lock) so it could just be an int.
Will make it an int.
bq. On 2011-04-28 00:21:00, Todd Lipcon wrote:
bq. > src/main/java/org/apache/hadoop/hbase/client/HTable.java, line 1361
bq. > <https://reviews.apache.org/r/643/diff/3/?file=16912#file16912line1361>
bq. >
bq. > these types of functions should be in try...finally. Otherwise
getRegionCachePrefetch("table that does not exist") would leak a connection.
bq. >
bq. > Ideally we would haven implementation of HConnection called
HConnectionRef which implements Closeable, so it would be the standard "get an
object, use it, close it" type pattern. Calling deleteConnection just feels
wrong.
I'll make sure the connection is cleaned up inside a finally block across the
board.
As far as the closeable is concerned, that was indeed implemented in an earlier
version, but I went back to using deleteConnection based on your comment about
having IOUtils.cleanUp method, which I think I must've misunderstood. In any
case, I can add the closeable back. Finally (no pun intended), I'll make sure
that if the close were to fail, that it wouldn't mask any exception in the try
block, if any.
- Karthick
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/643/#review593
-----------------------------------------------------------
On 2011-04-28 00:13:52, 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-28 00:13:52)
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
70affa0
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/mapred/TableOutputFormat.java
80284bb
bq. src/main/java/org/apache/hadoop/hbase/mapreduce/TableOutputFormat.java
05d9395
bq.
src/main/java/org/apache/hadoop/hbase/mapreduce/replication/VerifyReplication.java
ed88bfa
bq. src/main/java/org/apache/hadoop/hbase/master/HMaster.java 250a8cf
bq. src/main/java/org/apache/hadoop/hbase/master/ServerManager.java 04befe9
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/master/TestClockSkewDetection.java
915cdf6
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
