[jira] [Commented] (HBASE-18126) Increment class
[ https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16039429#comment-16039429 ] Ted Yu commented on HBASE-18126: Addendum pushed into branch. > 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 > Fix For: HBASE-14850 > > Attachments: 18126.v6.txt, 18126.v7.txt, 18126.v8.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)
[jira] [Commented] (HBASE-18126) Increment class
[
https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16039403#comment-16039403
]
Enis Soztutar commented on HBASE-18126:
---
Thanks Ted. Can you please do a little addendum to extract the new tests in
bytes-util-test to a different function. They do not belong in
TestToStringBinary. You should do something like:
{code}
TEST(TestBytesUtil, TestToStringToInt64) {
int64_t num = 761235;
EXPECT_EQ(num, BytesUtil::ToInt64(BytesUtil::ToString(num)));
...
}
{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
> Fix For: HBASE-14850
>
> Attachments: 18126.v6.txt, 18126.v7.txt, 18126.v8.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)
[jira] [Commented] (HBASE-18126) Increment class
[ https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16037847#comment-16037847 ] Ted Yu commented on HBASE-18126: Added tests in bytes-util-test for large positive number and small negative number. Adopted the macro from boost - I need to switch the order of the two blocks since the first block in previous patch was for small endian. Thanks for the review. > 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, 18126.v7.txt, 18126.v8.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)
[jira] [Commented] (HBASE-18126) Increment class
[
https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16037615#comment-16037615
]
Enis Soztutar commented on HBASE-18126:
---
Can you please address these:
- This should be named ToInt64()
- Forgot to ask last time, can you please add some basic tests for
BytesUtil::ToInt64(). Like convert some longs (min, max, 0, some arbitrary
numbers) back and forth between string and int64_t.
- Instead of defining the IsBigEndian(), you can use {{#include
}} and do something like:
{code}
std::string BytesUtil::ToString(int64_t val) {
std::string res;
#if BOOST_ENDIAN_BIG_BYTE || BOOST_ENDIAN_BIG_WORD
int64_t mask = 0xff00u;
for (int i = 56; i >= 0; i -= 8) {
res += (int8_t) ((val & mask) >> i);
mask = mask >> 8;
}
#else
for (int i = 7; i > 0; i--) {
res += (int8_t) (val & 0xffu);
val = val >> 8;
}
res += (int8_t) val;
#endif
return res;
}
{code}
above code not tested though.
Other than these +1.
> 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, 18126.v7.txt, 18126.v8.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)
[jira] [Commented] (HBASE-18126) Increment class
[
https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16035180#comment-16035180
]
Enis Soztutar commented on HBASE-18126:
---
bq. This is the interesting part: though DebugString() says 1 associated cell,
I get failed check if I try to access cell 0
In HBase RPC, cells can be carried over the wire without the PB encoding as the
RPC payload. Cells written this way will not be in the Result objects PB data
structure. Rather, there will be the counter {{associated_cell_count}} which
shows how many cells to read from the wire after the RPC response. The
CellCodecs (specifically KeyValueCodec) are used to read these cells which is
carried via Response::cell_scanner.
If you are using {{ResponseConverter::ToResult}} the cell-scanner should
automatically be used without having to do anything. Are you using this API or
something else?
> 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, 18126.v7.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)
[jira] [Commented] (HBASE-18126) Increment class
[
https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16034810#comment-16034810
]
Ted Yu commented on HBASE-18126:
This is the interesting part: though DebugString() says 1 associated cell, I
get failed check if I try to access cell 0:
{code}
I0602 02:53:35.203034 19696 response-converter.cc:53] FromMutateResponse:result
{
associated_cell_count: 1
stale: false
}
[libprotobuf FATAL /usr/local/include/google/protobuf/repeated_field.h:1408]
CHECK failed: (index) < (current_size_):
{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, 18126.v7.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)
[jira] [Commented] (HBASE-18126) Increment class
[
https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16033901#comment-16033901
]
Ted Yu commented on HBASE-18126:
Debugging why mutate response conversion doesn't work.
Here is debug string for the mutate response received:
{code}
I0601 23:23:41.431021 11620 response-converter.cc:52] FromMutateResponse:result
{
associated_cell_count: 1
stale: false
}
{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, 18126.v7.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)
[jira] [Commented] (HBASE-18126) Increment class
[
https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=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}} of the
incremented value.
{code}
+folly::Future 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)
[jira] [Commented] (HBASE-18126) Increment class
[ https://issues.apache.org/jira/browse/HBASE-18126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=16029876#comment-16029876 ] Enis Soztutar commented on HBASE-18126: --- Are you gonna do Append here as well, or that will be a different jira? Can do either way. > 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 > > These Increment objects are used by the Table implementation to perform > increment operation. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
