[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[
https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15948054#comment-15948054
]
Enis Soztutar commented on HBASE-17771:
---
Unfortunately, there is a bug introduced in {{Row::CheckRow}} causing the
get-test to fail. You are casting the size of the row to an {{int16_t}},
causing the integer overload when row.size is greater than
{{std::numeric_limits::max()}}. Anyway, good that we have the get-test
which caught it.
{code}
void CheckRow(const std::string ) {
const int16_t kMaxRowLength = std::numeric_limits::max();
int64_t row_length = row.size();
{code}
I've fixed the code and will push this to the branch.
> [C++] Classes required for implementation of BatchCallerBuilder
> ---
>
> Key: HBASE-17771
> URL: https://issues.apache.org/jira/browse/HBASE-17771
> Project: HBase
> Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Fix For: HBASE-14850
>
> Attachments: HBASE-17771.HBASE-14850.v1.patch,
> HBASE-17771.HBASE-14850.v2.patch, HBASE-17771.HBASE-14850.v3.patch,
> HBASE-17771.HBASE-14850.v4.patch, HBASE-17771.HBASE-14850.v5.patch,
> HBASE-17771.HBASE-14850.v6.patch
>
>
> Separating depedencies of BatchCallerBuilder.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[
https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15946636#comment-15946636
]
Hadoop QA commented on HBASE-17771:
---
| (/) *{color:green}+1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 14m 39s
{color} | {color:blue} Docker mode activated. {color} |
| {color:blue}0{color} | {color:blue} shelldocs {color} | {color:blue} 0m 10s
{color} | {color:blue} Shelldocs was not available. {color} |
| {color:green}+1{color} | {color:green} hbaseanti {color} | {color:green} 0m
0s {color} | {color:green} Patch does not have any anti-patterns. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m 0s
{color} | {color:green} The patch does not contain any @author tags. {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m 2s
{color} | {color:green} HBASE-14850 passed with JDK v1.8.0_121 {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m 1s
{color} | {color:green} HBASE-14850 passed with JDK v1.7.0_121 {color} |
| {color:green}+1{color} | {color:green} hbaseprotoc {color} | {color:green} 0m
2s {color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m 1s
{color} | {color:green} the patch passed with JDK v1.8.0_121 {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m 1s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m 1s
{color} | {color:green} the patch passed with JDK v1.7.0_121 {color} |
| {color:green}+1{color} | {color:green} cc {color} | {color:green} 0m 1s
{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} shellcheck {color} | {color:green} 0m
4s {color} | {color:green} There were no new shellcheck issues. {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s {color} | {color:green} The patch has no whitespace issues. {color} |
| {color:green}+1{color} | {color:green} hadoopcheck {color} | {color:green}
29m 18s {color} | {color:green} Patch does not cause any errors with Hadoop
2.4.0 2.4.1 2.5.0 2.5.1 2.5.2 2.6.1 2.6.2 2.6.3 2.7.1. {color} |
| {color:green}+1{color} | {color:green} hbaseprotoc {color} | {color:green} 0m
3s {color} | {color:green} the patch passed {color} |
| {color:black}{color} | {color:black} {color} | {color:black} 44m 25s {color}
| {color:black} {color} |
\\
\\
|| Subsystem || Report/Notes ||
| Docker | Client=1.11.2 Server=1.11.2 Image:yetus/hbase:date2017-03-29 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/12860998/HBASE-17771.HBASE-14850.v6.patch
|
| JIRA Issue | HBASE-17771 |
| Optional Tests | cc compile shellcheck shelldocs |
| uname | Linux a92087c76a4c 3.13.0-105-generic #152-Ubuntu SMP Fri Dec 2
15:37:11 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | nobuild |
| Personality |
/home/jenkins/jenkins-slave/workspace/PreCommit-HBASE-Build/component/dev-support/hbase-personality.sh
|
| git revision | HBASE-14850 / 915d89f |
| shellcheck | v0.4.6 |
| modules | C: . U: . |
| Console output |
https://builds.apache.org/job/PreCommit-HBASE-Build/6249/console |
| Powered by | Apache Yetus 0.3.0 http://yetus.apache.org |
This message was automatically generated.
> [C++] Classes required for implementation of BatchCallerBuilder
> ---
>
> Key: HBASE-17771
> URL: https://issues.apache.org/jira/browse/HBASE-17771
> Project: HBase
> Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Fix For: HBASE-14850
>
> Attachments: HBASE-17771.HBASE-14850.v1.patch,
> HBASE-17771.HBASE-14850.v2.patch, HBASE-17771.HBASE-14850.v3.patch,
> HBASE-17771.HBASE-14850.v4.patch, HBASE-17771.HBASE-14850.v5.patch,
> HBASE-17771.HBASE-14850.v6.patch
>
>
> Separating depedencies of BatchCallerBuilder.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[ https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15946385#comment-15946385 ] Enis Soztutar commented on HBASE-17771: --- Thanks Sudeep. Do you mind rebasing the patch to the current branch head. > [C++] Classes required for implementation of BatchCallerBuilder > --- > > Key: HBASE-17771 > URL: https://issues.apache.org/jira/browse/HBASE-17771 > Project: HBase > Issue Type: Sub-task >Reporter: Sudeep Sunthankar >Assignee: Sudeep Sunthankar > Fix For: HBASE-14850 > > Attachments: HBASE-17771.HBASE-14850.v1.patch, > HBASE-17771.HBASE-14850.v2.patch, HBASE-17771.HBASE-14850.v3.patch, > HBASE-17771.HBASE-14850.v4.patch, HBASE-17771.HBASE-14850.v5.patch > > > Separating depedencies of BatchCallerBuilder. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[ https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15946314#comment-15946314 ] Sudeep Sunthankar commented on HBASE-17771: --- Hi Enis, Lets commit this. Let me implement read and write locks at the AsyncBatchRpcRetryingCaller level. Also we would have the Row class in the repo which is required by HBASE-15894 Thanks. > [C++] Classes required for implementation of BatchCallerBuilder > --- > > Key: HBASE-17771 > URL: https://issues.apache.org/jira/browse/HBASE-17771 > Project: HBase > Issue Type: Sub-task >Reporter: Sudeep Sunthankar >Assignee: Sudeep Sunthankar > Fix For: HBASE-14850 > > Attachments: HBASE-17771.HBASE-14850.v1.patch, > HBASE-17771.HBASE-14850.v2.patch, HBASE-17771.HBASE-14850.v3.patch, > HBASE-17771.HBASE-14850.v4.patch, HBASE-17771.HBASE-14850.v5.patch > > > Separating depedencies of BatchCallerBuilder. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[ https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15945729#comment-15945729 ] Enis Soztutar commented on HBASE-17771: --- [~sudeeps] do you want me to commit this, and we do a follow up jira for the thread safety, or you want to do it in this patch. Either works, just let me know which is more convenient for you. > [C++] Classes required for implementation of BatchCallerBuilder > --- > > Key: HBASE-17771 > URL: https://issues.apache.org/jira/browse/HBASE-17771 > Project: HBase > Issue Type: Sub-task >Reporter: Sudeep Sunthankar >Assignee: Sudeep Sunthankar > Fix For: HBASE-14850 > > Attachments: HBASE-17771.HBASE-14850.v1.patch, > HBASE-17771.HBASE-14850.v2.patch, HBASE-17771.HBASE-14850.v3.patch, > HBASE-17771.HBASE-14850.v4.patch, HBASE-17771.HBASE-14850.v5.patch > > > Separating depedencies of BatchCallerBuilder. -- This message was sent by Atlassian JIRA (v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[
https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15943794#comment-15943794
]
Enis Soztutar commented on HBASE-17771:
---
bq. Added mutexes in case of writes. For reads, we are not using mutexes as of
now as all the methods are called by const objects. Further we are returning a
const reference so no changes will happen this way.
According to
https://stackoverflow.com/questions/10568969/c-stl-vector-iterator-vs-indexes-access-and-thread-safety
and
https://twiki.cern.ch/twiki/bin/view/CMSPublic/FWMultithreadedThreadSafeDataStructures
this is not true.
Anyway, there are two approaches we can take:
- Use a mutex at the Caller level. We can address perf problems if it becomes
a problem.
- Use concurrent maps from the Intels TBB library (follys concurrent maps are
extremely limiting).
You can change this to VLOG(3) or something:
{code}
LOG(INFO) << "GetResults:" << multi_resp->ShortDebugString();
{code}
Otherwise the rest looks pretty good.
> [C++] Classes required for implementation of BatchCallerBuilder
> ---
>
> Key: HBASE-17771
> URL: https://issues.apache.org/jira/browse/HBASE-17771
> Project: HBase
> Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Fix For: HBASE-14850
>
> Attachments: HBASE-17771.HBASE-14850.v1.patch,
> HBASE-17771.HBASE-14850.v2.patch, HBASE-17771.HBASE-14850.v3.patch,
> HBASE-17771.HBASE-14850.v4.patch
>
>
> Separating depedencies of BatchCallerBuilder.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[
https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15937128#comment-15937128
]
Enis Soztutar commented on HBASE-17771:
---
Thanks for the updated patch.
- Remove the commented-out code here:
{code}
+ std::shared_ptr stat_; // =
std::make_shared();
{code}
- Is this conditional coming from your custom testing? Need to remove it.
{code}
+ if (action_num % 3 == 0)
+pb_action->set_allocated_get(nullptr);
+ else
+pb_action->set_allocated_get(pb_get.release());
{code}
- Use {{auto unique = std::make_unique(*pb_req)}} here:
{code}
+ auto unique = std::unique_ptr(new Request(*pb_req));
{code}
- And maybe replace
{code}
+ auto region_specifier = new hbase::pb::RegionSpecifier();
+ RequestConverter::SetRegion(region_name, region_specifier);
+ auto pb_region_action = pb_msg->add_regionaction();
+ pb_region_action->set_allocated_region(region_specifier);
{code}
with something like:
{code}
+ auto pb_region_action = pb_msg->add_regionaction();
+ RequestConverter::SetRegion(region_name, pb_region_action->mutable_region());
{code}
- Are these statements even necessary? I don't think so:
{code}
+ auto unique = std::unique_ptr(new Request(*pb_req));
+ VLOG(8) << "pb_req Addr:-" << pb_req.get() << "; unique Addr:-" <<
unique.get();
+ multi_req = std::move(unique);
+ VLOG(8) << "multi_req Addr:-" << multi_req.get();
{code}
Seems coming from debugging.
- Rename this var to be {{multi_response}}.
+ auto multi_results = std::make_unique();
- Can we have some consistency in naming these (and related methods):
{code}
MultiResponse::Add()
RegionRequest::add_action()
RegionResult::add_result()
ServerRequest::AddAction()
{code}
Either it should be add_action and add_result or AddAction and AddResult.
- Are you gonna handle thread safety in this patch, or in a follow up?
> [C++] Classes required for implementation of BatchCallerBuilder
> ---
>
> Key: HBASE-17771
> URL: https://issues.apache.org/jira/browse/HBASE-17771
> Project: HBase
> Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Fix For: HBASE-14850
>
> Attachments: HBASE-17771.HBASE-14850.v1.patch,
> HBASE-17771.HBASE-14850.v2.patch, HBASE-17771.HBASE-14850.v3.patch
>
>
> Separating depedencies of BatchCallerBuilder.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[
https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15930967#comment-15930967
]
Enis Soztutar commented on HBASE-17771:
---
- Either return {{const Row &}} or {{std::shared_ptr}} here:
{code}
+ Row *action() const { return action_.get(); }
{code}
- {{RegionRequest:: set_action()}} should be named {{AddAction}} or
{{add_action()}}, because it is not setting an action, but adding to a list. In
terms of naming, we are trying to follow:
https://google.github.io/styleguide/cppguide.html#Function_Names. Specifically,
ordinary methods are named like {{Foo::BarBaz()}}, but "accessor" methods for
get / set are named {{Foo::bar()}}. That is why {{RegionRequest::actions()}} is
fine. Let's follow these conventions going forward.
- We should not be doing dynamic cast to Get like this here:
{code}
+if (auto pget = dynamic_cast(action)) {
+ auto pb_get = RequestConverter::ToGet(*pget);
{code}
The reason is that when we add Put objects as actions, then we have to use
{{typeid}} and will then have to depend on RTTI. Maybe we should templetize the
{{Action}} class instead of using Row? Anyway, let's leave this for a follow up
issue, because I do not want to hold on this patch for longer.
- Why is this returning a Future? You should just return
{{std::unique_ptr}}.
{code}
+folly::Future RequestConverter::ToMultiRequest(
{code}
- remove this:
{code}
+ // return
folly::makeFuture(std::move(multi_results));
{code}
- We are not gonna use region load statistics for the foreseeable future, so
you can ignore that for future patches. Feel free to keep these here since we
already have them.
{code}
+hbase::pb::MultiRegionLoadStats stats = multi_resp->regionstatistics();
{code}
- Here you are converting a unique_ptr from the factory to a shared_ptr:
{code}
+auto unique_result = ToResult(roe.result(), resp.cell_scanner());
+result = std::make_shared(*unique_result);
{code}
The efficient way to do this is something like:
{code}
+auto unique_result = ToResult(roe.result(), resp.cell_scanner());
+result = std::move(unique_result);
{code}
- {{ResponseType}} is not used, remove.
- {{+ explicit MultiResponse(const std::string& region_name);}} does not look
correct (multi response is for multiple regions). And the constructor does not
seem to be used, remove if so.
- We had talked (offline) about the maps in {{RegionRequest}} and
{{ServerRequest}} being Concurrent maps. It is pretty important to have these
data structures to be thread-safe. Remember that in the multi-request scatter /
gather logic, the Batch caller will be scheduling RPCs to multiple servers, and
each action maybe retried by the retry timer. That is why we have to have some
kind of thread-safety in these. You can either use Concurent maps from folly,
or, use mutexes for accessing these maps.
> [C++] Classes required for implementation of BatchCallerBuilder
> ---
>
> Key: HBASE-17771
> URL: https://issues.apache.org/jira/browse/HBASE-17771
> Project: HBase
> Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Fix For: HBASE-14850
>
> Attachments: HBASE-17771.HBASE-14850.v1.patch,
> HBASE-17771.HBASE-14850.v2.patch
>
>
> Separating depedencies of BatchCallerBuilder.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
[jira] [Commented] (HBASE-17771) [C++] Classes required for implementation of BatchCallerBuilder
[
https://issues.apache.org/jira/browse/HBASE-17771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15923033#comment-15923033
]
Enis Soztutar commented on HBASE-17771:
---
- Changes in {{core/BUCK}} undos the previous changes from HBASE-17728. This is
probably due to rebasing, but we need HBASE-17728.
- {{Action::OriginalIndex()}} should be named {{Action::original_index()}} and
{{Action::GetAction()}} should be {{Action::action()}}. Maybe in the other
classes as well (my reading of the conventions is that if the class method is
an accessor to a class field, you can name it like foo_bar(), instead of
FooBar().
- Class {{MultiAction}} is not needed?. Seems to be coming from the
AsyncProcess based impl. I've checked the patch at HBASE-17576, not used there
as well.
- Remove commented out code:
{code}
+ /*static std::shared_ptr GetResults(const
std::shared_ptr& req,
+ std::unique_ptr resp);*/
The non-commented out version takes Request by raw-pointer which is probably
not what we want.
{code}
- The CellScanner::Advance() might return false as well for the below code:
{code}
-while (cell_scanner->Advance()) {
+int num_cells = 0;
+while (num_cells != result.associated_cell_count()) {
+ cell_scanner->Advance();
{code}
So, I think you should check for that, and throw an exception if we wanted to
read that many cells, but the CellScanner::Advance() is returning false.
- Are you gonna address this? {{+// TODO Revisit this class once}}
- RegionRequest::ActionList is a shared_ptr, while
ServerRequest::ActionsByRegion is not. I think you don't need the shared_ptr in
RegionRequest::ActionList, especially because you are returning via {{const
&}}.
- RequestConverter::ToGet() is good, but it is duplicating the code. Maybe make
it so that ToGetRequest() uses that.
> [C++] Classes required for implementation of BatchCallerBuilder
> ---
>
> Key: HBASE-17771
> URL: https://issues.apache.org/jira/browse/HBASE-17771
> Project: HBase
> Issue Type: Sub-task
>Reporter: Sudeep Sunthankar
>Assignee: Sudeep Sunthankar
> Fix For: HBASE-14850
>
> Attachments: HBASE-17771.HBASE-14850.v1.patch
>
>
> Separating depedencies of BatchCallerBuilder.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
