hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Enis Soztutar (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-17576) [C++] Implement request retry mechanism over RPC for Multi calls.
Date Mon, 10 Apr 2017 08:57:41 GMT

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

Enis Soztutar commented on HBASE-17576:

bq. I believe we should take this approach instead of adding it to Response object.
Please create a different issue for this and attach the patch there, and let's get it committed

Please address this comment as well from before: 
std::vector<std::shared_ptr<hbase::Get>> is already an instance of std::vector<std::shared_ptr<Row>>,
no? You should not need to copy the vector.

- This code still returns an empty vector when it is executed: 
+  std::vector<FutureResult> results;
+  caller->Call().then([caller, &results](std::unique_ptr<std::vector<FutureResult>>
presults) {
+    for (auto itr = presults->begin(); itr != presults->end(); ++itr) {
+      results.push_back(std::move(*itr));
+    }
+    return folly::unit;
+  });
+  return results;
Remember that when you chain lamdba's like this to the Future object using {{then()}} method,
they will not get executed when the caller returns. Thus, the results vector will only get
populated asynchronously which is not what we want. 

- This is not a unit test as it stands. Please don't use simple_client at all. In this test,
there is already a mini-cluster. Please write the data in the unit test by copying code from
simple-client.cc or code from {{TEST_F(ClientTest, PutGet)}} in the same file. You may have
to rebase your patch for this. 
+TEST(Client, Gets) {

- for synchornizing the action2erros_ DS. This is similar to the handling done at Java API.
The point of above comment was that, given the more coarse grained lock (which is the {{access_}}
mutex that you have in the patch), there should not be any need for a second-level mutex.
You should make it so that only 1 mutex is used for both success or error cases. 

Coming to the way that you use {{access_}} is also not correct. You should make it more coarse
grained (for example inside OnError, etc), and it does not protect all maps that are concurrent.
You should instead acquire the lock in most of the methods at the beginning, and hold the
lock until the end. Also, acquire the locks at each of the lambda functions as well since
those lambdas are changing the maps. Remember that the lamdas execute after the actual method
returns, so by the time lambda executes, the original method who "injected" the lambda is
gone (hence its mutexes are de-allocated). 

> [C++] Implement request retry mechanism over RPC for Multi calls.
> -----------------------------------------------------------------
>                 Key: HBASE-17576
>                 URL: https://issues.apache.org/jira/browse/HBASE-17576
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Sudeep Sunthankar
>            Assignee: Sudeep Sunthankar
>         Attachments: HBASE-17576.HBASE-14850.v1.patch, HBASE-17576.HBASE-14850.v2.patch,
HBASE-17576.HBASE-14850.v3.patch, HBASE-17576.HBASE-14850.v4.patch, HBASE-17576.HBASE-14850.v5.patch,
HBASE-17576.HBASE-14850.v6.patch, HBASE-17576.HBASE-14850.v7.patch, HBASE-17576.HBASE-14850.v8.patch
> This work is based on top of HBASE-17465. Multi Calls will be based on this.

This message was sent by Atlassian JIRA

View raw message