You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by "Ken Weiner (JIRA)" <ji...@apache.org> on 2009/07/14 21:53:14 UTC
[jira] Created: (HBASE-1655) Usability improvements to HTablePool
Usability improvements to HTablePool
------------------------------------
Key: HBASE-1655
URL: https://issues.apache.org/jira/browse/HBASE-1655
Project: Hadoop HBase
Issue Type: Improvement
Components: client
Reporter: Ken Weiner
A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
I will be submitting a patch that contains the following changes to HTablePool:
* Remove constructors that were not used.
* Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
* Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
* Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Ken Weiner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ken Weiner updated HBASE-1655:
------------------------------
Status: Patch Available (was: Open)
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Attachments: HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Assigned: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Gray reassigned HBASE-1655:
------------------------------------
Assignee: Ken Weiner
Ken, added you as a contributor so you can now assign yourself issues and such.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Ken Weiner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12731574#action_12731574 ]
Ken Weiner commented on HBASE-1655:
-----------------------------------
- Even though TreeMap uses the comparator rather than the equals method to compare keys, using a byte[] as the key seems to break the contract of a java.util.Map. The last paragraph of the [Map API|http://java.sun.com/javase/6/docs/api/java/util/Map.html] says that impls are free to avoid calling equals() but the containsKey(Object key) method should "return true if and only if this map contains a mapping for a key k such that (key==null ? k==null : key.equals(k))". As you mentioned, you'd need a byte[] wrapper in order to be compliant. So a byte[] key will work in a TreeMap (and not in a HashMap), but we'd break the Map contract. Seems better to just use String and HashMap which works well and satisfies the Map contract.
- Allowing both the static methods and the public construction would bring me back to the complaint I had originally with this class. :) It becomes confusing for a user trying to quickly understand how he/she is supposed to interact with this class. I guess this can all be explained away with documentation, but, it doesn't feel "good".
- Also, yes, there would be a problem with re-instantiating the static map multiple times, but this could be prevented by implementing a Singleton pattern so that the static access operates on an internal singleton instance with a member Map rather than a static Map.
- I agree that there is no reason to construct HBC's for each invocation. An internal HBC instance can be constructed and reused.
- Is there a checkstyle.xml file for HBase? That would make it easy to check my code for formatting problems before submitting patches.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12731505#action_12731505 ]
Jonathan Gray commented on HBASE-1655:
--------------------------------------
I agree on most of your points... We have an internal HTable pooler (we call it HBaseRef) that works more like you describe.
- When using byte[] in a HashMap it will actually end up acting on the address of the byte[] rather than the bytes contained within. You have to use a wrapping class that would yield hashCode() and equals() methods if you want byte[] as the key in a HashMap.
- We can switch it all to String, but underneath these are byte[]s so it's just more consistent to actually use byte[]. If we can keep the API as small as possible, let's just include both.
- There's no reason we can't do both of these things. Make the constructor public and you can instantiate them manually. I guess in your framework you might end up re-instantiating the static map multiple times? The reason I like it is because you don't have to pass the pool around your code, you can always just reference the static instance of everything. My vote would be to try to do both. If we end up
- No worries on the little stuff... all part of the initiation process ;)
Do you think there is a way we could retain the static stuff and still allow HTablePool to be instantiable? We should just write a nice long class comment once we settle on the design with the different ways to use it, etc...
I would vote to keep the static stuff as short as possible, maybe just 2 static methods to get an HTable (w/ and w/o the size param). No statics to get the HTablePools. You have to instantiate then and that's where you would be able to set manual HBC and such.
The other thing is to make sure we aren't constructing HBCs constantly.
Good stuff, Ken. Thanks for figuring this.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Gray updated HBASE-1655:
---------------------------------
Resolution: Fixed
Hadoop Flags: [Reviewed]
Status: Resolved (was: Patch Available)
Committed after receiving +1s from apurtell (original author) and ken. New test added passes.
Removed HTable comments during commit.
Thanks, Ken!
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655-v3.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Gray updated HBASE-1655:
---------------------------------
Attachment: HBASE-1655-v2-partial.patch
This doesn't include any stargate stuff, only issues there are the tab issues, reordering, and whether to expose the pools or not.
I'm not sure we want to remove the ability to work with HTablePool directly or not.
This patch re-publics a bunch of stuff, cuts down on the API quite a bit, and reduces re-instantiation of HBC when getting pools that already exist and will always reuse the original when getting tables. We should definitely keep that.
Otherwise the changes are open to discussion, I'm not particularly sold on my own patch, just providing it so we have a point of reference for discussion.
The biggest thing to figure out is whether we even expose HTablePool non-statically to the user. If not, we go down the path of a much longer easy-to-use API...
/me sleeping on it
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Ken Weiner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12732679#action_12732679 ]
Ken Weiner commented on HBASE-1655:
-----------------------------------
+1 on removing the comment about allowing nulls. Your rational makes sense to me.
Do I need to make a new patch, or can the commiter just remove the comments from HTable after applying the current patch?
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655-v3.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12731235#action_12731235 ]
Jonathan Gray commented on HBASE-1655:
--------------------------------------
Patch looks pretty good. Small nitpicky issues with some needless reorderings of stuff and also some tabbing oddities. Can be fixed on commit.
I remember why we're using TreeMap now instead of HashMap. HashMap with byte[] as a key does not work with default comparator, and you cannot pass one in. There is no real downside to using TreeMap unless you have hundreds or thousands of tables, and even then logarithmic on the client-side on the order of 100-1000 is negligible. More efficient in memory but a little dirtier on the GC... however this is almost exclusively client-side (and there's ever only one) so really makes no difference IMO. So, back to TreeMap.
The API seems to have grown quite a bit. Do we need all these permutations of getTable() ? Could we drop the String taking ones besides the simplest one? (This is why we don't support String in most of the HBase API now, leads to very long and ugly APIs)
The default getTable(byte [] tableName) also requires instantiating a new HBaseConfiguration() each time internally, even if we are reusing an existing HTable... Whether there is a significant overhead or not to that, we should avoid it when unnecessary.
Typical usage is probably to not supply your own HBC, so if someone wants that level of control, give them the ability to build the Pool themselves rather than expose the friendlier, higher-level methods with all the different ways to instantiate the pool.
Blah, this is better said in a new patch....
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Attachments: HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Ken Weiner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12731271#action_12731271 ]
Ken Weiner commented on HBASE-1655:
-----------------------------------
A few questions/comments on the comments:
- Why does the key to a HashMap need a comparator?
- I much prefer the type of String vs byte[] for the tableName throughout all the method signatures and as a key to the internal map, but I tried to keep it as-is because it seemed to fit more with other HBase code. Can we just change it to String throughout and go back to the HashMap?
- I am not a fan of the current API either. I'd much prefer that the HTablePool be instantiated by a client and we get rid of the static methods and static Map altogether. This fits in much more nicely for people like me who are using IoC containers like Spring. It also allows the ability to have multiple HTablePool instances, maybe each with their own configuration (which is currently just the max size).
- Sorry about the tab/spaces issue. I didn't clean it up carefully enough.
- Sorry about the reordering of imports. I use Eclipse which does this automatically and I was too lazy to try to restore the order back to the way it was. I'll try harder next time.
So what is the next step? Should I make a new patch with an HTablePool that is meant to be instantiated and w/o the static methods?
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Gray updated HBASE-1655:
---------------------------------
Fix Version/s: 0.20.0
Moved into 0.20.0, we need to have a concise/consistent story for this client-facing class.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Ken Weiner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ken Weiner updated HBASE-1655:
------------------------------
Attachment: HBASE-1655.patch
A patch with modified HTablePool is now attached.
I also modified code in the stargate package that was using HTablePool.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Attachments: HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Gray updated HBASE-1655:
---------------------------------
Attachment: HBASE-1655-HTablePool-commented.patch
Rewrite of, and commented version of HTablePool, worked on by Ken and I yesterday. Discussion to follow from Ken ;)
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "stack (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12731596#action_12731596 ]
stack commented on HBASE-1655:
------------------------------
.bq So a byte[] key will work in a TreeMap (and not in a HashMap), but we'd break the Map contract. Seems better to just use String and HashMap which works well and satisfies the Map contract.
Looking at the TreeMap equals implementation, yes, it breaks the above contract for Map.
We're impure (insert lots of sack cloth and ashes, self-flagellation, etc., here -- smile).
We are zealous about using byte [] everywhere. As Jon allows above, client-side, we can relax some.
IMO, we should not be allowing public construction and static methods.
+1 on Singleton to prevent multiple instantiations.
There is no checkstyle (maybe there is one up in hadoop). In general 80 characters per line and two spaces for tab. Anything else we'll take. Don't worry about it too much.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jonathan Gray updated HBASE-1655:
---------------------------------
Status: In Progress (was: Patch Available)
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Attachments: HBASE-1655-v2-partial.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Ken Weiner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ken Weiner updated HBASE-1655:
------------------------------
Attachment: HBASE-1655-v3.patch
Attaching patch file reflecting several discussions about the best design for the HTablePool.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655-v3.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12732217#action_12732217 ]
Jonathan Gray commented on HBASE-1655:
--------------------------------------
I'm +1 on the changes described. I'm not a big fan of putting a big comment in the HTable constructor javadoc about being able to pass it a null.
IMO Something like that belongs in a // comment in the code, not exposed in the javadoc of the most client-facing class we have. It's for developers, not users. Users who pass a null HBC will soon learn that this will not work, why do they care about unit testing?
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655-v3.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Updated: (HBASE-1655) Usability improvements to HTablePool
Posted by "Ken Weiner (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ken Weiner updated HBASE-1655:
------------------------------
Status: Patch Available (was: In Progress)
I've submitted another patch, HBASE-1655-v3.patch, that reflects a different approach taken to improve HTablePool. In the new approach, there will be no static methods at all. Instead, the normal usage of the class will be to instantiate an HTablePool directly and use the getTable and putTable methods to get an HTable and return an HTable to the pool. The pool class will automatically instantiate a new HTable in the case that no HTable for the given table name is in the pool OR if the internal pool for that table name is full (has more elements than its configured max size). The default max size is Integer.MAX_VALUE.
While we lose the convenience of calling static methods, we gain a design with less code that is easy to understand, easy to use in an IoC container, and easy to unit test.
This patch also includes a small modification to HTable that allows an instantiation to HTable with a null HBaseConfiguration object. This makes it possible to unit test classes such as HTablePool that internally use an HTable without requiring an environment in which the HTable can get and use a connection.
Finally, this patch includes modifications to the stargate package reflecting the new HTablePool API.
Let me know if this is suitable for inclusion in the project. Thanks to Jon Gray and Stack for their help and input.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655-v3.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Jonathan Gray (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12732688#action_12732688 ]
Jonathan Gray commented on HBASE-1655:
--------------------------------------
Will take care of it on commit.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655-v3.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (HBASE-1655) Usability improvements to HTablePool
Posted by "Andrew Purtell (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/HBASE-1655?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12732219#action_12732219 ]
Andrew Purtell commented on HBASE-1655:
---------------------------------------
+1
Also, I agree with jgray about leaving out mention of allowing nulls in the HTable constructor.
> Usability improvements to HTablePool
> ------------------------------------
>
> Key: HBASE-1655
> URL: https://issues.apache.org/jira/browse/HBASE-1655
> Project: Hadoop HBase
> Issue Type: Improvement
> Components: client
> Reporter: Ken Weiner
> Assignee: Ken Weiner
> Fix For: 0.20.0
>
> Attachments: HBASE-1655-HTablePool-commented.patch, HBASE-1655-v2-partial.patch, HBASE-1655-v3.patch, HBASE-1655.patch
>
>
> A discussion on the HBase user mailing list (http://markmail.org/thread/7leeha56ny5mwecg) led to some suggested improvements for the org.apache.hadoop.hbase.client.HTablePool class.
> I will be submitting a patch that contains the following changes to HTablePool:
> * Remove constructors that were not used.
> * Change access to remaining contstructor from public to private to enforce use of the static factory method getPool.
> * Change internal map from TreeMap to HashMap because I couldn't see any reason it needed to be sorted.
> * Remove HBaseConfiguration and tableName member variables since they aren't really properties of the pool itself. They are associated with the HTable that should get instantiated when one is requested from the pool, but not already there.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.