You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hbase.apache.org by Jonathan Gray <jg...@apache.org> on 2010/11/11 00:19:26 UTC
Review Request: HBASE-3211 Key only filter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1208/
-----------------------------------------------------------
Review request for hbase, stack and Kannan Muthukkaruppan.
Summary
-------
Adds a new filter, KeyOnlyFilter. The idea is that this will make it so only the key portion of all the KVs are returned. Could imagine a few use cases where you just need the keys/index not the values. We have one where we have giant rows with big values and want to just get the qualifiers/versions w/o values.
Adds a new method in KeyValue, convertToKeyOnly(). From javadoc:
/**
* Converts this KeyValue to only contain the key portion (the value is
* changed to be null). This method does a full copy of the backing byte
* array and does not modify the original byte array of this KeyValue.
* <p>
* This method is used by {@link KeyOnlyFilter} and is an advanced feature of
* KeyValue, proceed with caution.
*/
This addresses bug HBASE-3211.
http://issues.apache.org/jira/browse/HBASE-3211
Diffs
-----
trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 1033617
trunk/src/main/java/org/apache/hadoop/hbase/filter/KeyOnlyFilter.java PRE-CREATION
trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1033617
trunk/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java 1033617
trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java 1033617
Diff: http://review.cloudera.org/r/1208/diff
Testing
-------
Test of the KV method added to TestKeyValue. Test of KeyOnlyFilter added to TestFilter. Both passing.
Thanks,
Jonathan
Re: Review Request: HBASE-3211 Key only filter
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1208/#review1903
-----------------------------------------------------------
trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java
<http://review.cloudera.org/r/1208/#comment6125>
my concern is far more down to earth here, the problem is that KeyValue is an immutable object that _sometimes_ mutates. Having this function call:
public KeyValue getKeyOnly() {
// insert implementation that doesnt modify this here
}
would be better.
- Ryan
On 2010-11-10 15:19:26, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1208/
> -----------------------------------------------------------
>
> (Updated 2010-11-10 15:19:26)
>
>
> Review request for hbase, stack and Kannan Muthukkaruppan.
>
>
> Summary
> -------
>
> Adds a new filter, KeyOnlyFilter. The idea is that this will make it so only the key portion of all the KVs are returned. Could imagine a few use cases where you just need the keys/index not the values. We have one where we have giant rows with big values and want to just get the qualifiers/versions w/o values.
>
> Adds a new method in KeyValue, convertToKeyOnly(). From javadoc:
>
> /**
> * Converts this KeyValue to only contain the key portion (the value is
> * changed to be null). This method does a full copy of the backing byte
> * array and does not modify the original byte array of this KeyValue.
> * <p>
> * This method is used by {@link KeyOnlyFilter} and is an advanced feature of
> * KeyValue, proceed with caution.
> */
>
>
> This addresses bug HBASE-3211.
> http://issues.apache.org/jira/browse/HBASE-3211
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 1033617
> trunk/src/main/java/org/apache/hadoop/hbase/filter/KeyOnlyFilter.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java 1033617
>
> Diff: http://review.cloudera.org/r/1208/diff
>
>
> Testing
> -------
>
> Test of the KV method added to TestKeyValue. Test of KeyOnlyFilter added to TestFilter. Both passing.
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: HBASE-3211 Key only filter
Posted by Jonathan Gray <jg...@apache.org>.
> On 2010-11-10 15:25:07, Ryan Rawson wrote:
> > trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java, line 1199
> > <http://review.cloudera.org/r/1208/diff/1/?file=17147#file17147line1199>
> >
> > I'm torn here, this is making the implementation easy, but KeyValues have been immutable to date. While no one shares KeyValues between threads or scanners, and ideally no one should, this seems dangerous.
> >
> >
It doesn't actually touch the original byte[] so does not actually destroy/mutate the underlying data in any way. Agreed it's still potentially "dangerous" but that's why I've added the nice warning message in javadoc :)
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1208/#review1891
-----------------------------------------------------------
On 2010-11-10 15:19:26, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1208/
> -----------------------------------------------------------
>
> (Updated 2010-11-10 15:19:26)
>
>
> Review request for hbase, stack and Kannan Muthukkaruppan.
>
>
> Summary
> -------
>
> Adds a new filter, KeyOnlyFilter. The idea is that this will make it so only the key portion of all the KVs are returned. Could imagine a few use cases where you just need the keys/index not the values. We have one where we have giant rows with big values and want to just get the qualifiers/versions w/o values.
>
> Adds a new method in KeyValue, convertToKeyOnly(). From javadoc:
>
> /**
> * Converts this KeyValue to only contain the key portion (the value is
> * changed to be null). This method does a full copy of the backing byte
> * array and does not modify the original byte array of this KeyValue.
> * <p>
> * This method is used by {@link KeyOnlyFilter} and is an advanced feature of
> * KeyValue, proceed with caution.
> */
>
>
> This addresses bug HBASE-3211.
> http://issues.apache.org/jira/browse/HBASE-3211
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 1033617
> trunk/src/main/java/org/apache/hadoop/hbase/filter/KeyOnlyFilter.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java 1033617
>
> Diff: http://review.cloudera.org/r/1208/diff
>
>
> Testing
> -------
>
> Test of the KV method added to TestKeyValue. Test of KeyOnlyFilter added to TestFilter. Both passing.
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: HBASE-3211 Key only filter
Posted by Ryan Rawson <ry...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1208/#review1891
-----------------------------------------------------------
trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java
<http://review.cloudera.org/r/1208/#comment6117>
I'm torn here, this is making the implementation easy, but KeyValues have been immutable to date. While no one shares KeyValues between threads or scanners, and ideally no one should, this seems dangerous.
- Ryan
On 2010-11-10 15:19:26, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1208/
> -----------------------------------------------------------
>
> (Updated 2010-11-10 15:19:26)
>
>
> Review request for hbase, stack and Kannan Muthukkaruppan.
>
>
> Summary
> -------
>
> Adds a new filter, KeyOnlyFilter. The idea is that this will make it so only the key portion of all the KVs are returned. Could imagine a few use cases where you just need the keys/index not the values. We have one where we have giant rows with big values and want to just get the qualifiers/versions w/o values.
>
> Adds a new method in KeyValue, convertToKeyOnly(). From javadoc:
>
> /**
> * Converts this KeyValue to only contain the key portion (the value is
> * changed to be null). This method does a full copy of the backing byte
> * array and does not modify the original byte array of this KeyValue.
> * <p>
> * This method is used by {@link KeyOnlyFilter} and is an advanced feature of
> * KeyValue, proceed with caution.
> */
>
>
> This addresses bug HBASE-3211.
> http://issues.apache.org/jira/browse/HBASE-3211
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 1033617
> trunk/src/main/java/org/apache/hadoop/hbase/filter/KeyOnlyFilter.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java 1033617
>
> Diff: http://review.cloudera.org/r/1208/diff
>
>
> Testing
> -------
>
> Test of the KV method added to TestKeyValue. Test of KeyOnlyFilter added to TestFilter. Both passing.
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: HBASE-3211 Key only filter
Posted by Jonathan Gray <jg...@apache.org>.
> On 2010-11-10 15:24:52, stack wrote:
> > Looks fine to me. That kv copy is ugly but what else can you do?
Definitely can't modify the original buffer, so it's the only choice.
In this case, it's not a huge deal because we'll do these allocations, return the result, and then immediately be done with the memory and will have no references to it. Should be okay on GC.
One potential optimization would be to do one big rewrite of the KVs at the end rather as we go. Instead of allocating individual byte[] for each KV, you could potentially do one big byte[] behind all the key-only KVs. This gets way more complicated and I'm not sure it's worth it. Was going for minimal approach.
In the filter unit test, I'm also going add an additional assert on commit (and verifying still passes). The test verifies the values are not the same but we should actually explicitly also assert that the value is 0 length.
Thanks!
- Jonathan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1208/#review1890
-----------------------------------------------------------
On 2010-11-10 15:19:26, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1208/
> -----------------------------------------------------------
>
> (Updated 2010-11-10 15:19:26)
>
>
> Review request for hbase, stack and Kannan Muthukkaruppan.
>
>
> Summary
> -------
>
> Adds a new filter, KeyOnlyFilter. The idea is that this will make it so only the key portion of all the KVs are returned. Could imagine a few use cases where you just need the keys/index not the values. We have one where we have giant rows with big values and want to just get the qualifiers/versions w/o values.
>
> Adds a new method in KeyValue, convertToKeyOnly(). From javadoc:
>
> /**
> * Converts this KeyValue to only contain the key portion (the value is
> * changed to be null). This method does a full copy of the backing byte
> * array and does not modify the original byte array of this KeyValue.
> * <p>
> * This method is used by {@link KeyOnlyFilter} and is an advanced feature of
> * KeyValue, proceed with caution.
> */
>
>
> This addresses bug HBASE-3211.
> http://issues.apache.org/jira/browse/HBASE-3211
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 1033617
> trunk/src/main/java/org/apache/hadoop/hbase/filter/KeyOnlyFilter.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java 1033617
>
> Diff: http://review.cloudera.org/r/1208/diff
>
>
> Testing
> -------
>
> Test of the KV method added to TestKeyValue. Test of KeyOnlyFilter added to TestFilter. Both passing.
>
>
> Thanks,
>
> Jonathan
>
>
Re: Review Request: HBASE-3211 Key only filter
Posted by st...@duboce.net.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://review.cloudera.org/r/1208/#review1890
-----------------------------------------------------------
Ship it!
Looks fine to me. That kv copy is ugly but what else can you do?
- stack
On 2010-11-10 15:19:26, Jonathan Gray wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://review.cloudera.org/r/1208/
> -----------------------------------------------------------
>
> (Updated 2010-11-10 15:19:26)
>
>
> Review request for hbase, stack and Kannan Muthukkaruppan.
>
>
> Summary
> -------
>
> Adds a new filter, KeyOnlyFilter. The idea is that this will make it so only the key portion of all the KVs are returned. Could imagine a few use cases where you just need the keys/index not the values. We have one where we have giant rows with big values and want to just get the qualifiers/versions w/o values.
>
> Adds a new method in KeyValue, convertToKeyOnly(). From javadoc:
>
> /**
> * Converts this KeyValue to only contain the key portion (the value is
> * changed to be null). This method does a full copy of the backing byte
> * array and does not modify the original byte array of this KeyValue.
> * <p>
> * This method is used by {@link KeyOnlyFilter} and is an advanced feature of
> * KeyValue, proceed with caution.
> */
>
>
> This addresses bug HBASE-3211.
> http://issues.apache.org/jira/browse/HBASE-3211
>
>
> Diffs
> -----
>
> trunk/src/main/java/org/apache/hadoop/hbase/KeyValue.java 1033617
> trunk/src/main/java/org/apache/hadoop/hbase/filter/KeyOnlyFilter.java PRE-CREATION
> trunk/src/main/java/org/apache/hadoop/hbase/io/HbaseObjectWritable.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/TestKeyValue.java 1033617
> trunk/src/test/java/org/apache/hadoop/hbase/filter/TestFilter.java 1033617
>
> Diff: http://review.cloudera.org/r/1208/diff
>
>
> Testing
> -------
>
> Test of the KV method added to TestKeyValue. Test of KeyOnlyFilter added to TestFilter. Both passing.
>
>
> Thanks,
>
> Jonathan
>
>