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 Thu, 27 Apr 2017 23:18:04 GMT

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

Enis Soztutar commented on HBASE-17576:
---------------------------------------

- From a coding practice, this is not how we check preconditions: 
{code}
+  if (conn_) {
+    location_cache_ = conn_->location_cache();
+    rpc_client_ = conn_->rpc_client();
+    cpu_pool_ = conn_->cpu_executor();
+  } else {
+    throw std::runtime_error("Passed null AsyncConnection. Unexpected.");
+  }
{code}
You should use CHECK macros from glog like this: 
CHECK(conn_ != nullptr); 
- Same thing for the below line (and elsewhere as well): 
{code}
if ((nullptr == location_cache_) || (nullptr == rpc_client_) || (nullptr == cpu_pool_))
{code}

- It is fine to not fix the retries in this patch: 
{code}
// TODO This gives segfault @ present, when retried
{code}
Please do a follow up patch. I'll commit HBASE-17800 shortly which should help with this.


- When formatting the code with bin/format-code.sh, it is interesting that your version of
clang-format does it slightly differently. Are you running inside the docker env? Have you
upgraded any of the LLVM toolchain? This constant change in formatting makes the patches harder
to read. 
- Remove these commented out code (and others if there are any): 
{code}
+//#include "core/async-rpc-retrying-caller-factory.h"
{code}
{code}
+  const /*std::vector<TryResult>*/auto tresults = collectAll(*fresults.get()).get(
{code}
{code}
+  // typedef BatchCallerBuilder GenenericThisType;
{code}

- Simple client does not write test2 and testextra columns, why are you adding them to the
list of gets: 
{code}
+  gets.push_back(std::make_shared<hbase::Get>(hbase::Get("test2")));
+  gets.push_back(std::make_shared<hbase::Get>(hbase::Get("testextra")));
{code}

- Result->DebugString() already prints out the Cell information, no? Why are you doing
extra logging inside this if: 
{code}
+      if(!result->IsEmpty()) {
{code}

- No need for checking the exception type in table.cc: 
{code}
+      if (result.exception().is_compatible_with<hbase::RetriesExhaustedException>())
{
{code} 
We need to re-throw the exception to the application if any coming from one of the Get results.


Will do more review in the next version. 

> [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,
HBASE-17576.HBASE-14850.v9.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)

Mime
View raw message