> On 2010-10-24 20:05:55, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java, > > line 195 > > <http://review.cloudera.org/r/1088/diff/2/?file=15905#file15905line195> > > > > adding trailing spaces > >
that's my patch removing trailing spaces > On 2010-10-24 20:05:55, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java, > > line 201 > > <http://review.cloudera.org/r/1088/diff/2/?file=15905#file15905line201> > > > > adding more trailing spaces, your ide should have a feature to strip > > these same here... i'm on the right :) > On 2010-10-24 20:05:55, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java, > > line 495 > > <http://review.cloudera.org/r/1088/diff/2/?file=15909#file15909line495> > > > > yeah you cant compare against memstoreTS because if you have this in > > here you wont be able to ever increment values that were inserted into the > > future. you'd just leave them there and continually see it in the 'get' > > part and then in this code bit leave it in place and create a new KV that > > is masked by the future KV. > > > > It won't be possible for that insert to be part of an uncommitted > > change because of the rowlock however. So no atomic-rules will have been > > broken. i'm not sure i follow. am i doing it wrong? this should be exactly the same as what's there. also, we are breaking the atomic-rules across the Increment. Each column is not broken but across them it is (for reads not writes). It seems like we could use RWCC though for increments. I think it's fine that if you're using increment you can never write data into it w/ another API (or really, with manual timestamps). > On 2010-10-24 20:05:55, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java, > > line 377 > > <http://review.cloudera.org/r/1088/diff/2/?file=15909#file15909line377> > > > > I'm not sure how this code is optional for your new 'upsert', here are > > some use cases that I found painful: > > > > - when the KV is in snapshot we can create new KVs in memstore with the > > same TS. This means you have a dup and before we had this new 'dup' code > > it ruined counts badly. > > - the Math.max() bit it to ensure the new KV isnt being created in the > > past a bit and accidently duplicating a timestamp inside the snapshot. what do you mean by optional? there shouldn't be any real difference. this code is basically the exact same code that was there but now pulled into a method that can be reused. > On 2010-10-24 20:05:55, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java, > > line 466 > > <http://review.cloudera.org/r/1088/diff/2/?file=15909#file15909line466> > > > > im not sure i like the name upsert, it is too rdbms-y for me. > > > > I need to poke more at this, but i prever the matchingRow() call, it > > encapsulates the getRowOffset junk, which leaks wayyy too much all over the > > place. > > > > Sure, could use those calls. Not even sure why I changed this, wrote that part of this patch a few weeks ago. And it's an "update if it exists, insert if not" operation which I think of as an upsert operation. Open to whatever tho. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.cloudera.org/r/1088/#review1641 ----------------------------------------------------------- On 2010-10-24 19:29:07, Jonathan Gray wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.cloudera.org/r/1088/ > ----------------------------------------------------------- > > (Updated 2010-10-24 19:29:07) > > > Review request for hbase, stack and khemani. > > > Summary > ------- > > Adds a new Increment class that allows multiple columns (each w/ own > increment amount) in a single row being incremented in one call. > > The big wins here are being able to do multiple columns in a row in a single > RPC and having it be appended/synced to the WAL in a single call. > > The current trade-off is that you lose atomicity to readers (ie. this does > not currently use RWCC). Eventually it could but for the current use case I > am building this for, it's okay like this. > > > This addresses bug HBASE-2946. > http://issues.apache.org/jira/browse/HBASE-2946 > > > Diffs > ----- > > trunk/src/main/java/org/apache/hadoop/hbase/client/HTable.java 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/client/HTableInterface.java > 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/client/Increment.java > PRE-CREATION > trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java > 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/ipc/HRegionInterface.java > 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java > 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/HRegionServer.java > 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java > 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 1026930 > trunk/src/main/java/org/apache/hadoop/hbase/rest/client/RemoteHTable.java > 1026930 > trunk/src/test/java/org/apache/hadoop/hbase/client/TestFromClientSide.java > 1026930 > > Diff: http://review.cloudera.org/r/1088/diff > > > Testing > ------- > > Added TestFromClientSide.testIncrement() which adds some client-side tests of > Increment (and mixing w/ original icv call). That passes and most the way > through a test suite run. > > > Thanks, > > Jonathan > >
