> On 2010-07-12 13:09:10, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java, > > line 425 > > <http://review.hbase.org/r/257/diff/4/?file=2332#file2332line425> > > > > is this idiomatic? I dont think I've seen this particular pattern > > before? > > Pranav Khaitan wrote: > This is a utility method in Writables and it does exactly the task we > want to get done here. There is an alternate method getWritable which also > returns an object of type Writable. Do you think it would be better to use > that?
ok i see, then this should be ok > On 2010-07-12 13:09:10, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 933 > > <http://review.hbase.org/r/257/diff/4/?file=2328#file2328line933> > > > > This doesnt appear to be germane to the issue at hand. It shouldn't > > appear in this patch. > > Pranav Khaitan wrote: > This method is required for this issue to check if a delete is a Column > of Family delete. It is used in TimeRangeTracker. If you suggest not adding > this method, I can write this code at the place I am using it at? ok my bad. this is fine then. > On 2010-07-12 13:09:10, Ryan Rawson wrote: > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java, > > line 136 > > <http://review.hbase.org/r/257/diff/4/?file=2333#file2333line136> > > > > This doesnt seem germane to the issue at hand... Can we not do this > > here? > > Pranav Khaitan wrote: > I did this to pass Timerange information to ShouldSeek(). An alternative > possibility is to pass TimeRange as an additional parameter. We had a > discussion about this in our team here and we thought that since Scan > contains all information, it is a neater way of passing than adding more > arguments. That way, in future if we add any more filtering techniques at > this level, we wont have to change the interface. > > Do you suggest making it two different functions: shouldSeek(row, > columns) and shouldSeek(timeRange) ? > > Alternately, it can be combined into one function like shouldSeek(isGet, > row, columns, timeRange) ok, lets keep it as then. - Ryan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://review.hbase.org/r/257/#review357 ----------------------------------------------------------- On 2010-07-09 15:28:53, Pranav Khaitan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://review.hbase.org/r/257/ > ----------------------------------------------------------- > > (Updated 2010-07-09 15:28:53) > > > Review request for hbase, Nicolas, Jonathan Gray, Karthik Ranganathan, and > Kannan Muthukkaruppan. > > > Summary > ------- > > Every memstore and store file will have a minimum and maximum timestamp > associated with it. If the range of timestamps we are searching for doesn't > overlap with the range for a particular file, we can skip searching it and > save time. > > Would significantly improve the performance for timestamp range queries. > Particularly useful when most of the reads are for recent entries and the > older files can be safely skipped. > > Addresses HBASE-2265 JIRA. > > This diff includes fixing some minor bugs like KeyValueHeap used to throw an > uncaught exception when size of scanner set was zero. > > Internal review done by Jonathan and Kannan. > > > This addresses bug HBASE-2265. > http://issues.apache.org/jira/browse/HBASE-2265 > > > Diffs > ----- > > trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 959782 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/KeyValueHeap.java > 959782 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/MemStore.java > 959782 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 959782 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFile.java > 959782 > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreFileScanner.java > 959782 > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/StoreScanner.java > 959782 > > trunk/src/main/java/org/apache/hadoop/hbase/regionserver/TimeRangeTracker.java > PRE-CREATION > > trunk/src/test/java/org/apache/hadoop/hbase/client/TestMultipleTimestamps.java > PRE-CREATION > trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java > 960082 > trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStore.java > 959782 > trunk/src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreFile.java > 959782 > > Diff: http://review.hbase.org/r/257/diff > > > Testing > ------- > > All existing JUnit tests run successfully. More JUnit tests for Memstore, > StoreFile and Store added to test correctness with multiple timestamps. > > Conducted a test to measure the extra time required to keep track of min and > max timestamps while writing KeyValues. The comparison was done by entering > 1 Million KeyValues into memstore ten times with and without timestamp > tracking and then taking the average time for each of them. WAL was disabled > and no flushing was done during this test to minimize overheads. The average > time taken for entering 1M KeyValues into memstore without keeping track of > timestamp was 13.44 seconds while the average time when keeping track of > timestamps was 13.45 seconds. This shows that no significant overhead has > been added while keeping track of timestamps. > > > Thanks, > > Pranav > >
