[ 
https://issues.apache.org/jira/browse/HBASE-1304?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12707549#action_12707549
 ] 

stack commented on HBASE-1304:
------------------------------

toString format needs to change in Delete, Get, etc., to match how we toString 
elsewhere using ruby-style for maps/arrays -- just for consistency sake.  
Ruby-style is pretty much how java-style works representing Maps and Lists and 
it matches shell.

Lets just drop RowLock from the new objects and use a long for lockid 
altogether?  Thats all it is and one less object.  Also io package not 
referencing item in client package.

Below is odd especially when Get does not implement Map
{code}
+  public Set<Map.Entry<byte[],TreeSet<byte[]>>> entrySet() {
+    return this.familyMap.entrySet();
+  }
{code}

Check for null array here:

{code}
+  public Result(KeyValue [] kvs) {
+    this.row = kvs[0].getRow();
+    this.kvs = kvs;
+  }
{code}

Does Result get instantiated server-side?  Would be nice if it wasn't; if 
Clients made it, if the Server passed back List<KV>.  Would save on Result 
instantiation and creation (and duplication) of a row in constructor (maybe 
getRow could be lazy.... don't create one unless its called).

Put some spaces in here so its more readable:

{code}
+  public SortedMap<byte[],SortedMap<byte[],SortedMap<Long,byte[]>>> getMap() {
{code}

Seems a pity creating a TreeMap when usual case is single family.   Later we 
can optimize and only create the TreeMap on addition of second family?

Whats this:

{code}
+  /**
+   * Returns a {...@link RowResult}.
+   * @return a RowResult
+   */
+  public RowResult rowResult() {
+    return new RowResult();
+  //  return RowResult.createRowResult(kvs);
+  }
{code}

The below could be written as: "return this.kvs == null || this.kvs.length == 
0;"

{code}
+    if(this.kvs == null || this.kvs.length == 0) {
+      return true;
+    }
+    return false;
{code}

Fix these: "+    for(int i=0; i<length; i++) " -- add spaces.

Lots of common code.  Common base abstract class?

These should be static defines rather than created on each construction.

{code}
+    this.minStamp = Bytes.toBytes(0L);
+    this.maxStamp = Bytes.toBytes(Long.MAX_VALUE);
{code}

Regards TimeRange, are we for sure that most compares of timestamps are byte [] 
rather than long?  This class is written for byte [].  Surely compare of two 
longs is faster than creation of byte array representation and then byte 
compare?

These should be made on TimeRange creation?  Or at least cached?

{code}
+    long min = Bytes.toLong(minStamp);
{code}

Above is minor.  In general this TimeRange byte [] compare of longs seems like 
an optimization that'd never register its so small?

More to follow.



> 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.

Reply via email to