Here's a patch.

Did a test where I pre-create both the ExecutorService and the HConnection instance. With this it is actually faster to create a new HTable than to get one from HTablePool.

I'd be fine putting this in 0.92. It's not disruptive and entirely backwards compatible.

I prefer the previous formatting (for the next time):

-  public HTable(Configuration conf, final String tableName)
-  throws IOException {
+  public HTable(Configuration conf, final String tableName) throws IOException {
     this(conf, Bytes.toBytes(tableName));

(and its < 80 chars on a line).

Nice javadoc on the HTable constructor.

Don't you need to javadoc that conf has to be null if you pass a connection and pool else they'll not be used?

Hmmm... maybe its better to not take a conf at all? Else it could get confusing. Say you'll use the configuration that is in the passed HConnection? Looking at the way this new constructor runs, I could really mess w/ you passing null Configuration and null Connection. More checks I'd say.

Else patch looks good to me and +1 on getting it into 0.92.

Will undo the formatting (Eclipse did that with the HBase formatter).
Configuration is still used to get conf settings. (That strange null check for it was in there before - note sure what use the HTable is if null is passed for Configuration).

Was thinking about getting the conf from the HConnection as well, but then it needed some more refactoring and I wanted a simple patch.

Hmm... Let me see what I can come up with. The HConnection's conf and the passed conf could be different and that would be... Bad.

I think we should also check the Configuration from HConnection at the beginning of the ctor:

     if (conf == null) {
       this.scannerTimeout = 0;

If conf is passed and HConnection isn't null, we should check their equality by virtue of HConnectionKey.CONNECTION_PROPERTIES

Maybe we can put HConnectionKey in its own class file and expose equality check by comparing conf values for the elements of HConnectionKey.CONNECTION_PROPERTIES

If conf and HConnection's configuration aren't equal according to the above check, throw exception in HTable ctor.

I like the idea of just passing the HConnection in that case better.

How about this?
The idea here is that either you the system do the "right thing", or you take over both HConnection and ExecutorService management.

Patch v2 is cleaner.

Getting the tests run.

Oh and Stack was right on the mailing list, currently the only way to get a handle to an HCI is by calling HCM.getConnection(). HCI is not accessible to the client.

Could add a public createConnection(Configuration) method to HCM to completely bypass the connection caching... It's not really needed for our purposes, but might be good for better isolation (i.e. what if somebody also uses HTable with the old constructors, the Connection might get cleaned up and closed).

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12504032/4805-v2.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

-1 tests included. The patch doesn't appear to include any new or modified tests.
Please justify why no new tests are needed for this patch.
Also please list what manual steps were performed to verify this patch.

-1 javadoc. The javadoc tool appears to have generated -163 warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

-1 findbugs. The patch appears to introduce 52 new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.client.TestScannerTimeout
org.apache.hadoop.hbase.client.TestFromClientSide
org.apache.hadoop.hbase.master.TestDistributedLogSplitting

Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/278//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/278//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/278//console

This message is automatically generated.

Could add a public createConnection(Configuration) method to HCM to completely bypass the connection caching... It's not really needed for our purposes....

I've been tempted to do this a bunch of times but have not yet had a good enough reason (recently I changed internals of HCM so could get at them from same package to help w/ mocking – so I could insert a mocked HConnection under HTable). I'd say leave it as is till we have a good reason.

+1 on patch

Set this.connection to null after you close it when you commit the patch?

+      if (this.connection != null) {
+        this.connection.close();

New patch. Fixes test failure (configuration returned from connection, is not necessarily the same that was used to retrieve the connection)

Setting configuration is explicitly now in addition to connection.

Also, that pointed the way towards needing truly unmanaged HConnections. Added a new createConnection(conf) method to HCM (with appropriate warnings in the Javadoc) to get an unmanaged HConnection, along with some tests.

@stack... Just saw your comment now. v3 adds a createConnection to HCM along with a flag to HConnectionImplementation to declare whether it is managed or not. I think otherwise it might get to fickle if there is code that uses "managed" HTables mixed with code that uses "unmanaged" HTables.

Let me know what you think. I can remove that part of the patch again, if it is too intrusive.

I don't think I follow why this.connection should be set to null.

I don't think I follow why this.connection should be set to null.

So we don't double close if HTable#close is called twice (not sure what would happen if we did call close on HConnection twice).

Regards:

+   * Use this with caution, your application <strong>must</strong> manage the
+   * connection's life cycle explicitly.

'Life-cycle' sounds exotic; do you mean the caller is responsible for calling close on the created HConnection?

If so, amend the javadoc on commit.

+1

I see... Double close on HConnection would decrease the Connection's reference count, so I can fold this in here.
I'll add that, change the Javadoc and post the patch (to get a safety test run )

Actually closing a closed HTable is no-op:

  public void close() throws IOException {
    if (this.closed) {
      return;
    }
    ...
    this.closed = true;
  }

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12504089/4805-v3.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 6 new or modified tests.

-1 javadoc. The javadoc tool appears to have generated -163 warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

-1 findbugs. The patch appears to introduce 52 new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.master.TestDistributedLogSplitting

Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/282//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/282//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/282//console

This message is automatically generated.

TestDistributedLogSplitting passes locally. So I think this good to go. I'll fix the java doc on commit.

@Ted and @stack, any further comments.

I'm thinking maybe this should only go into trunk, because of the changes in HCM (still OK with 0.92 if nobody else is worried).

Patch v3 looks good.

+  public HTable(final byte[] tableName, final HConnection connection, 
+      final ExecutorService pool) throws IOException {
+    assert connection != null && pool != null;

I think we should also verify that the connection isn't closed in the ctor.

Ah, good call. Assert good enough? Or should it throw an exception? (asserts are disabled in production code)

I think throwing IllegalStateException would be good if connection is closed in ctor.

Used IllegalArgumentException instead (also used in validatePut(...)).

Minor comment:

+    assert connection != null && pool != null;
+    if (connection.isClosed()) {

Since assertion is not effective at runtime, we should convert the check on line 1.
Also, we should verify that (pool != null && !pool.isShutdown())

Agreed... Got rid of asserts.

+1 if tests pass.

Good work Lars.

+1 on patch (minor nit for future, isManaged is not a good name for a boolean – managed should be the name and isManaged the name of the method that tests if managed is set – for future).

Oh, +1 on commit to 0.92 too.

Committed to 0.92 and trunk.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12504130/4805-v4.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 6 new or modified tests.

-1 javadoc. The javadoc tool appears to have generated -163 warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

-1 findbugs. The patch appears to introduce 52 new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.master.TestDistributedLogSplitting
org.apache.hadoop.hbase.client.TestShell

Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/284//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/284//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/284//console

This message is automatically generated.

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12504136/4805-v5.txt
against trunk revision .

+1 @author. The patch does not contain any @author tags.

+1 tests included. The patch appears to include 6 new or modified tests.

-1 javadoc. The javadoc tool appears to have generated -163 warning messages.

+1 javac. The applied patch does not increase the total number of javac compiler warnings.

-1 findbugs. The patch appears to introduce 52 new Findbugs (version 1.3.9) warnings.

+1 release audit. The applied patch does not increase the total number of release audit warnings.

-1 core tests. The patch failed these unit tests:
org.apache.hadoop.hbase.client.TestShell

Test results: https://builds.apache.org/job/PreCommit-HBASE-Build/285//testReport/
Findbugs warnings: https://builds.apache.org/job/PreCommit-HBASE-Build/285//artifact/trunk/patchprocess/newPatchFindbugsWarnings.html
Console output: https://builds.apache.org/job/PreCommit-HBASE-Build/285//console

This message is automatically generated.

Integrated in HBase-TRUNK #2453 (See https://builds.apache.org/job/HBase-TRUNK/2453/)
HBASE-4805 Allow better control of resource consumption in HTable (Lars H)

larsh :
Files :

• /hbase/trunk/CHANGES.txt
• /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
• /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
• /hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java
• /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
• /hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java

Integrated in HBase-0.92 #142 (See https://builds.apache.org/job/HBase-0.92/142/)
HBASE-4805 Allow better control of resource consumption in HTable (Lars H)

larsh :
Files :

• /hbase/branches/0.92/CHANGES.txt
• /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HConnection.java
• /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HConnectionManager.java
• /hbase/branches/0.92/src/main/java/org/apache/hadoop/hbase/client/HTable.java
• /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
• /hbase/branches/0.92/src/test/java/org/apache/hadoop/hbase/client/TestHCM.java

Do you plan to backport it to 0.90.x branch as well or I should rather wait for 0.92.x+?

Cheers,
Krystian