[jira] [Commented] (HBASE-18126) Increment class

["Enis Soztutar (JIRA)" <ji...@apache.org>]

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

Enis Soztutar commented on HBASE-18126:
---------------------------------------

Looks good overall. A couple of comments: 

- This should be named {{ToInt64()}} and should have a return value of int64_t. {{long}} has no guarantees on the number of bytes that represents the value, while int64_t is exactly 64bits. Since we are interoperating with Java whose ints and longs are 32 and 64 respectively, we should always use {{int32_t}} or {{int64_t}}, and never int / long for these kind of APIs. See the patch for HBASE-17220 for examples. 
{code}
+long BytesUtil::ToLong(std::string str)
{code}
also change the type for the variable {{l}}. 

- You have to check the length before accessing things like this: 
{code}
+      l ^= bytes[i];
{code}
I was checking the cost of std::string::c_str() it seems in most implementations, it is just returning the internal pointer. So using it should be fine. 

- You need to do this reverse because of endianness. I think this code should work with both little and big endian machines: 
+    std::reverse(res.begin(), res.end());
Instead of reversing at the end, let's do two loops after detecting the endianness of the machine. There may be a more optimal way using reinterpret_cast or something, but these can be optimized later. 

- In the delete patch the corresponding method was named {{DeleteToMutateRequest}}, but here it is {{RequestConverter::ToIncrementRequest}}. Let's stick to one naming scheme (either change the name of the Delete method, or change this name). 

- Increment should return a {{std::shared_ptr<hbase::Result>}} of the incremented value. 
{code}
+folly::Future<folly::Unit> RawAsyncTable::Increment(const hbase::Increment& incr) {
{code}


> Increment class
> ---------------
>
>                 Key: HBASE-18126
>                 URL: https://issues.apache.org/jira/browse/HBASE-18126
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Ted Yu
>            Assignee: Ted Yu
>         Attachments: 18126.v6.txt
>
>
> These Increment objects are used by the Table implementation to perform increment operation.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)