[jira] [Commented] (HBASE-18061) [C++] Fix retry logic in multi-get calls

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

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

Enis Soztutar commented on HBASE-18061:
---------------------------------------

Thanks Sudeep for the updated patch. 
Can you please clean up this part: 
{code}
             if (search != actions_by_server.end()) {
+              // Multiple regions might be hosted on the same server (ServerName)
+              // Check if a Region is already present in the map or not. If not add it
+              // auto actions_by_region_name = search->second->actions_by_region();
+              auto itr = search->second->actions_by_region().find(region_loc->region_name());
+              if (itr != search->second->actions_by_region().end()) {
                 search->second->AddActionsByRegion(region_loc, action);
               } else {
-              // Create new key
+                // Create new key for the region location region_loc
+                search->second = std::make_shared<ServerRequest>(region_loc);
+                search->second->AddActionsByRegion(region_loc, action);
+                // search->second = server_request;
+              }
+            } else {
+              // Create new key for this ServerName
{code} 

- Can you also cleap up the VLOG's a little bit. We can use VLOG(5) for very excessive stuff, VLOG(1) for most of the exceptions, and anything in between. VLOG(8) is too much. 
- Please remove all commented out code like this: 
{code}
+//#include "connection/request.h"
..
//LOG(INFO) << "tresults size :- " << tresults.size();
{code}
- Did you run bin/format-code.sh and make lint ? 
- remove these: 
{code}
//TODO This shouldn't be in THROW, but we are getting incorrect RL's at the moment
{code}
Some of these tests were expected to fail, no? 
- You need to enable this test? 
{code}
+TEST_F(AsyncBatchRpcRetryTest, TestFailWithException) {
+  std::shared_ptr<AsyncRegionLocatorBase> region_locator(
+      std::make_shared<MockWrongRegionAsyncRegionLocator>(5));
+  EXPECT_ANY_THROW(runMultiTest(region_locator, "table3"));
+  //TODO will be in EXPECT_ANY_THROW
+  // Not failing.
+  //runMultiTest(region_locator, "table3");
+}
{code}
- Undo this: 
{code}
-    test_util->StartMiniCluster(2);
+    test_util->StartMiniCluster(10);
{code}
- Please go over the patch again, and make sure that there is no commented-out code lying around. 
- This test is good {{+TEST_F(ClientTest, MultiGetsWithRegionSplits)}}, but there is a lot of code duplication. Maybe move the logic to a method, and call it from the MutlGets and MultiGetsWithRegionSplits tests. 


> [C++] Fix retry logic in multi-get calls
> ----------------------------------------
>
>                 Key: HBASE-18061
>                 URL: https://issues.apache.org/jira/browse/HBASE-18061
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Enis Soztutar
>            Assignee: Sudeep Sunthankar
>             Fix For: HBASE-14850
>
>         Attachments: HBASE-18061.HBASE-14850.v1.patch, HBASE-18061.HBASE-14850.v3.patch, HBASE-18061.HBASE-14850.v5.patch
>
>
> HBASE-17576 adds multi-gets. There are a couple of todos to fix in the retry logic, and some unit testing to be done for the multi-gets. We'll do these in this issue. 



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)