[
https://issues.apache.org/jira/browse/HBASE-1304?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12707587#action_12707587
]
stack commented on HBASE-1304:
------------------------------
No class comment on QueryMatcher. What is it?
All ^Ms still. Need to remove.
Does this enum need to be public? Shouldn't it be package private? Also, doc
using javadoc rather than comments:
{code}
+ // ColumnMatcher.match() return codes
+ public static enum MatchCode {
+ INCLUDE, // Include in result
+ SKIP, // Do not include in result
+ NEXT, // Move to next StoreFile
+ DONE // Query done, return
+ }
{code}
Is the comment for next right? We just made use of this enum in Memcache.
Data members should be final.
Things like this.deletes should be moved out of constructor and instead make
the declaration and assignment happen in the one statement.
Why not use the defines that are in KV rather than do below:
{code}
+ offset += Bytes.SIZEOF_INT + Bytes.SIZEOF_INT;
{code}
This is usually written as ret < 0 rather than as + if(ret <= -1) {
There are methods in KV for doing some of this stuff that is in match. Having
it here and in KV makes it hard if we want to change KV internals. Use the KV
defines or just use the KV methods getKeyLength, getRowOffset, etc. There are
even versions where you can pass stuff you've already calculated -- lengths and
offsets -- so the KV method doesn't do it again.
Where do we accumulate deletes? In this match I don't see that we are adding
to the deletes structure.
Below could be written as return timestamp < oldestStamp instead of as:
{code}
+ if(timestamp < oldestStamp) {
+ return true;
+ }
+ return false;
{code}
If expired and memcache, do you just want to remove it? It'd be rare case I
suppose?
Could we early out here:
{code}
+ // Read from Memcache
+ this.memcache.get(matcher, result);
{code}
We don't check the return from this.memcache.get?
Rather than do this.storefiles.size() == 0, do isEmpty. storefiles is a
concurrentskipmaplist. size is expensive on this structures.
Do we have to make the List<HFileScanner> storefileScanners? Can't we just
process the store files one at a time as they come out of the descendingMap?
Whats advantage of preallocating scanners? They are used once only anyways?
Just do just-in-time allocation close to where they are used?
What is a StoreFileScanner? Members should be final.
Remove the ^Ms.
Again SKIP and NEXT seem off.
Isn't it possible that INCLUDE might want us to keep going in the store file?
Perhaps more answers to get (same up in memcache). Instead we break on INCLUDE.
What is a WildcardColumnTracker
Bytes is a Writable? Whats difference between Bytes and
ImmutableBytesWritable? Why is it Writable? Should we deprecate
ImmutableBytesWritable and use this instead?
Should use Bytes.toString() rather than new String when doing the toString for
Bytes. The former does utf8. Latter whatever the user locale.
parseColumn doesn't belong in Bytes. Put it in KeyValue?
There do not seem to be any new tests?
What do we have here? Does it work? What more is to be done? What you have
for an estimate before you have it all put back together? I see deletes still
needs to be worked through, I see scanners and puts still need to be done.
Seems like a bunch of work still to do? What you reckon lads?
> New client server implementation of how gets and puts are handled.
> -------------------------------------------------------------------
>
> Key: HBASE-1304
> URL: https://issues.apache.org/jira/browse/HBASE-1304
> Project: Hadoop HBase
> Issue Type: Improvement
> Affects Versions: 0.20.0
> Reporter: Erik Holstad
> Assignee: Jonathan Gray
> Priority: Blocker
> Fix For: 0.20.0
>
> Attachments: hbase-1304-v1.patch, HBASE-1304-v2.patch,
> HBASE-1304-v3.patch
>
>
> Creating an issue where the implementation of the new client and server will
> go. Leaving HBASE-1249 as a discussion forum and will put code and patches
> here.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
