[ 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)