You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Todd Lipcon <to...@cloudera.com> on 2010/06/15 01:30:30 UTC
Review Request: HBASE-2670. Memstore should retain multiple versions of a
row when memstore TS differs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/180/
-----------------------------------------------------------
Review request for hbase and Ryan Rawson.
Summary
-------
This changes the memstore comparator, improves the atomicity tests, and also fixes ICV to continue to have the right behavior even with the comparator change.
This addresses bug hbase-2670.
http://issues.apache.org/jira/browse/hbase-2670
Diffs
-----
src/main/java/org/apache/hadoop/hbase/KeyValue.java 71284cf
src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 2a0dcee
src/test/java/org/apache/hadoop/hbase/MultithreadedTestUtil.java 7c062d7
src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java 75f3c8b
src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java 9833d76
src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java 0566af7
Diff: http://review.hbase.org/r/180/diff
Testing
-------
Unit tests.
Thanks,
Todd
Re: Review Request: HBASE-2670. Memstore should retain multiple versions of
a row when memstore TS differs
Posted by Todd Lipcon <to...@cloudera.com>.
> On 2010-06-15 15:03:40, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 558
> > <http://review.hbase.org/r/180/diff/1/?file=1345#file1345line558>
> >
> > Is this going to be correct always? Cloning, we don't want the src memstoreTS?
yea, this clone is necessary so that incrementColumnValue() doesn't blow up to create a new memstore entry for every increment. We could put it in that code, but I think it's cleaner to just be part of clone. I think it's best in clone so that a foo.compareTo(foo.clone()) == 0 as an invariant.
> On 2010-06-15 15:03:40, stack wrote:
> > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1306
> > <http://review.hbase.org/r/180/diff/1/?file=1345#file1345line1306>
> >
> > What we going to do about N versions all of same r/f/q/ts but of different memstoreTS? We're not going to suppress them just yet? We're going to punt till hbase-1485? The multiple versions make it out to store files too?
Yea, leaving that off since the problem already exists and it's a bit of a heavy change.
> On 2010-06-15 15:03:40, stack wrote:
> > src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java, line 140
> > <http://review.hbase.org/r/180/diff/1/?file=1348#file1348line140>
> >
> > Prefix w/ 'TODO' to make this work-to-do more findable.
ah, I mean that we don't need to verify at this point because the writer hasn't actually written any rows yet! will clarify
- Todd
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/180/#review229
-----------------------------------------------------------
On 2010-06-14 16:30:30, Todd Lipcon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/180/
> -----------------------------------------------------------
>
> (Updated 2010-06-14 16:30:30)
>
>
> Review request for hbase and Ryan Rawson.
>
>
> Summary
> -------
>
> This changes the memstore comparator, improves the atomicity tests, and also fixes ICV to continue to have the right behavior even with the comparator change.
>
>
> This addresses bug hbase-2670.
> http://issues.apache.org/jira/browse/hbase-2670
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/KeyValue.java 71284cf
> src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 2a0dcee
> src/test/java/org/apache/hadoop/hbase/MultithreadedTestUtil.java 7c062d7
> src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java 75f3c8b
> src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java 9833d76
> src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java 0566af7
>
> Diff: http://review.hbase.org/r/180/diff
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Todd
>
>
Re: Review Request: HBASE-2670. Memstore should retain multiple versions of
a row when memstore TS differs
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/180/#review229
-----------------------------------------------------------
Ship it!
+1
I some minors in the below and then some questions but these should not get in way of a commit.
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<http://review.hbase.org/r/180/#comment1016>
Is this going to be correct always? Cloning, we don't want the src memstoreTS?
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<http://review.hbase.org/r/180/#comment1017>
What we going to do about N versions all of same r/f/q/ts but of different memstoreTS? We're not going to suppress them just yet? We're going to punt till hbase-1485? The multiple versions make it out to store files too?
src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java
<http://review.hbase.org/r/180/#comment1018>
Prefix w/ 'TODO' to make this work-to-do more findable.
src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java
<http://review.hbase.org/r/180/#comment1019>
Nice
src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java
<http://review.hbase.org/r/180/#comment1020>
Go ahead and remove then boss.
- stack
On 2010-06-14 16:30:30, Todd Lipcon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/180/
> -----------------------------------------------------------
>
> (Updated 2010-06-14 16:30:30)
>
>
> Review request for hbase and Ryan Rawson.
>
>
> Summary
> -------
>
> This changes the memstore comparator, improves the atomicity tests, and also fixes ICV to continue to have the right behavior even with the comparator change.
>
>
> This addresses bug hbase-2670.
> http://issues.apache.org/jira/browse/hbase-2670
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/KeyValue.java 71284cf
> src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 2a0dcee
> src/test/java/org/apache/hadoop/hbase/MultithreadedTestUtil.java 7c062d7
> src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java 75f3c8b
> src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java 9833d76
> src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java 0566af7
>
> Diff: http://review.hbase.org/r/180/diff
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Todd
>
>
Re: Review Request: HBASE-2670. Memstore should retain multiple versions of
a row when memstore TS differs
Posted by st...@duboce.net.
> On 2010-06-15 18:06:09, Ryan Rawson wrote:
> > src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1306
> > <http://review.hbase.org/r/180/diff/1/?file=1345#file1345line1306>
> >
> > the extra KVs will make it out to hfile, but considering the problem I just don't see a better solution right now. Perhaps collapsing extra version during the flush, but then the question is _which_ extra versions?
> >
> > Maybe this will lead us to using an extra timestamp in every KeyValue on disk and in memory, but I hope not.
.bq ...but then the question is _which_ extra versions?
For now, I'd think we'd keep most recently written.
.bq Maybe this will lead us to using an extra timestamp in every KeyValue on disk and in memory
Maybe. When we tease out deletes and how ts's are meant to work then we'll learn whether or not they are needed I'd imagine.
- stack
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/180/#review235
-----------------------------------------------------------
On 2010-06-14 16:30:30, Todd Lipcon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/180/
> -----------------------------------------------------------
>
> (Updated 2010-06-14 16:30:30)
>
>
> Review request for hbase and Ryan Rawson.
>
>
> Summary
> -------
>
> This changes the memstore comparator, improves the atomicity tests, and also fixes ICV to continue to have the right behavior even with the comparator change.
>
>
> This addresses bug hbase-2670.
> http://issues.apache.org/jira/browse/hbase-2670
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/KeyValue.java 71284cf
> src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 2a0dcee
> src/test/java/org/apache/hadoop/hbase/MultithreadedTestUtil.java 7c062d7
> src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java 75f3c8b
> src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java 9833d76
> src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java 0566af7
>
> Diff: http://review.hbase.org/r/180/diff
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Todd
>
>
Re: Review Request: HBASE-2670. Memstore should retain multiple versions of
a row when memstore TS differs
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.hbase.org/r/180/#review235
-----------------------------------------------------------
Ship it!
ok looks good. I'm also prepping a new way to do ICVs in a different issue (HBASE-2501).
src/main/java/org/apache/hadoop/hbase/KeyValue.java
<http://review.hbase.org/r/180/#comment1066>
the extra KVs will make it out to hfile, but considering the problem I just don't see a better solution right now. Perhaps collapsing extra version during the flush, but then the question is _which_ extra versions?
Maybe this will lead us to using an extra timestamp in every KeyValue on disk and in memory, but I hope not.
- Ryan
On 2010-06-14 16:30:30, Todd Lipcon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.hbase.org/r/180/
> -----------------------------------------------------------
>
> (Updated 2010-06-14 16:30:30)
>
>
> Review request for hbase and Ryan Rawson.
>
>
> Summary
> -------
>
> This changes the memstore comparator, improves the atomicity tests, and also fixes ICV to continue to have the right behavior even with the comparator change.
>
>
> This addresses bug hbase-2670.
> http://issues.apache.org/jira/browse/hbase-2670
>
>
> Diffs
> -----
>
> src/main/java/org/apache/hadoop/hbase/KeyValue.java 71284cf
> src/main/java/org/apache/hadoop/hbase/regionserver/Store.java 2a0dcee
> src/test/java/org/apache/hadoop/hbase/MultithreadedTestUtil.java 7c062d7
> src/test/java/org/apache/hadoop/hbase/TestAcidGuarantees.java 75f3c8b
> src/test/java/org/apache/hadoop/hbase/regionserver/TestMemStore.java 9833d76
> src/test/java/org/apache/hadoop/hbase/regionserver/TestStoreScanner.java 0566af7
>
> Diff: http://review.hbase.org/r/180/diff
>
>
> Testing
> -------
>
> Unit tests.
>
>
> Thanks,
>
> Todd
>
>