Mailing-List: contact issues-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Date: Mon, 10 Apr 2017 08:57:41 +0000 (UTC)
From: "Enis Soztutar (JIRA)" <jira@apache.org>
To: issues@hbase.apache.org
Message-ID: <JIRA.13039460.1485955715000.245825.1491814661600@Atlassian.JIRA>
In-Reply-To: <JIRA.13039460.1485955715000@Atlassian.JIRA>
References: <JIRA.13039460.1485955715000@Atlassian.JIRA> <JIRA.13039460.1485955715117@jira-lw-us.apache.org>
Subject: [jira] [Commented] (HBASE-17576) [C++] Implement request retry
 mechanism over RPC for Multi calls.
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
archived-at: Mon, 10 Apr 2017 08:57:48 -0000


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

Please address this comment as well from before: 
{quote}
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.
{quote}

- This code still returns an empty vector when it is executed: 
{code}
+  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;
{code}
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. 
{code}
+TEST(Client, Gets) {
{code}

- 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
(v6.3.15#6346)