[ https://issues.apache.org/jira/browse/HBASE-5229?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13200681#comment-13200681 ]
jirapos...@reviews.apache.org commented on HBASE-5229: ------------------------------------------------------ ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/3748/#review4822 ----------------------------------------------------------- A couple of nits and small implementation details, but overall looks pretty good. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java <https://reviews.apache.org/r/3748/#comment10588> I think is this unnecessary, javadoc should handle inheriting the docs. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java <https://reviews.apache.org/r/3748/#comment10587> or presplitting as is described in other documenttation. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiRowMutation.java <https://reviews.apache.org/r/3748/#comment10586> Probably want to wrap NOTE in <b> tags to call it out. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiRowMutation.java <https://reviews.apache.org/r/3748/#comment10590> A javadoc here might be nice to indicate that the nullary constructor is actually completely ok to use (as opposed to the more common state of being reserved for readFields). http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiRowMutation.java <https://reviews.apache.org/r/3748/#comment10585> Even though it uses protected structures doesn't mean that its necessarily thread safe. In fact, because it is using the standard ArrayList, there is no guarantee of safety. Either the class should be marked as not thread safe OR the mutations should be wrapped as a concurrent list. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/RowMutation.java <https://reviews.apache.org/r/3748/#comment10591> You really don't need to keep around the row anymore either because you can get that from the mutations as you already do mutateRows with MultiRowMutation. Its nice to store it, but is only going to be checked infrequently and saves you a little bit over the wire (which could add up, depending on row size). http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <https://reviews.apache.org/r/3748/#comment10592> Suprised this isn't a utility method in HRegion - it seems really useful. Maybe worth pulling out for general use. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <https://reviews.apache.org/r/3748/#comment10593> This isn't actually true, right? With multirow, you are actually going to lock more than one row (and the lockId null seems kind of a hack around that as it is always null, so far). http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java <https://reviews.apache.org/r/3748/#comment10594> nit: lockID rather than just lid would be slightly descriptive. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <https://reviews.apache.org/r/3748/#comment10595> But in the comments on the MultiRowMutation you push that checking off onto the RS, so no checking really happens then (except, I guess when you try to mutate rows on the region and it fails b/c those rows aren't there, but that seems kinda late for the check). http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java <https://reviews.apache.org/r/3748/#comment10596> Wow, this is ugly. Maybe we should consider some refactoring of this later? http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java <https://reviews.apache.org/r/3748/#comment10597> This class can get easily bloated as we add more types. Might be worth considering refactoring this out into its own test. - Jesse On 2012-02-03 19:59:55, Lars Hofhansl wrote: bq. bq. ----------------------------------------------------------- bq. This is an automatically generated e-mail. To reply, visit: bq. https://reviews.apache.org/r/3748/ bq. ----------------------------------------------------------- bq. bq. (Updated 2012-02-03 19:59:55) bq. bq. bq. Review request for hbase. bq. bq. bq. Summary bq. ------- bq. bq. This builds on HBASE-3584, HBASE-5203, and HBASE-5304. bq. bq. Multiple Rows can be locked and applied atomically as long as the application ensures that all rows reside in the same Region (by presplitting or a custom RegionSplitPolicy). bq. At SFDC we can use this to colocate subsets of a tenant's data and allow atomic operations over these subsets. bq. bq. Obviously this is an advanced features and this prominently called out in the Javadoc. bq. bq. bq. This addresses bug HBASE-5229. bq. https://issues.apache.org/jira/browse/HBASE-5229 bq. bq. bq. Diffs bq. ----- bq. bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/client/MultiRowMutation.java PRE-CREATION bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/coprocessor/CoprocessorHost.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java 1239953 bq. http://svn.apache.org/repos/asf/hbase/trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestAtomicOperation.java 1239953 bq. bq. Diff: https://reviews.apache.org/r/3748/diff bq. bq. bq. Testing bq. ------- bq. bq. Tests added to TestFromClientSide and TestAtomicOperation bq. bq. bq. Thanks, bq. bq. Lars bq. bq. > Explore building blocks for "multi-row" local transactions. > ----------------------------------------------------------- > > Key: HBASE-5229 > URL: https://issues.apache.org/jira/browse/HBASE-5229 > Project: HBase > Issue Type: New Feature > Components: client, regionserver > Reporter: Lars Hofhansl > Assignee: Lars Hofhansl > Fix For: 0.94.0 > > Attachments: 5229-multiRow-v2.txt, 5229-multiRow.txt, > 5229-seekto-v2.txt, 5229-seekto.txt, 5229.txt > > > HBase should provide basic building blocks for multi-row local transactions. > Local means that we do this by co-locating the data. Global (cross region) > transactions are not discussed here. > After a bit of discussion two solutions have emerged: > 1. Keep the row-key for determining grouping and location and allow efficient > intra-row scanning. A client application would then model tables as > HBase-rows. > 2. Define a prefix-length in HTableDescriptor that defines a grouping of > rows. Regions will then never be split inside a grouping prefix. > #1 is true to the current storage paradigm of HBase. > #2 is true to the current client side API. > I will explore these two with sample patches here. > -------------------- > Was: > As discussed (at length) on the dev mailing list with the HBASE-3584 and > HBASE-5203 committed, supporting atomic cross row transactions within a > region becomes simple. > I am aware of the hesitation about the usefulness of this feature, but we > have to start somewhere. > Let's use this jira for discussion, I'll attach a patch (with tests) > momentarily to make this concrete. -- This message is automatically generated by JIRA. If you think it was sent incorrectly, please contact your JIRA administrators: https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa For more information on JIRA, see: http://www.atlassian.com/software/jira