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-17771) [C++] Classes required for implementation of BatchCallerBuilder
Date Sat, 18 Mar 2017 01:15:41 GMT

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

Enis Soztutar commented on HBASE-17771:
---------------------------------------

- Either return {{const Row &}} or {{std::shared_ptr<Row>}} here: 
{code}
+  Row *action() const { return action_.get(); }
{code} 
- {{RegionRequest:: set_action()}} should be named {{AddAction}} or {{add_action()}}, because
it is not setting an action, but adding to a list. In terms of naming, we are trying to follow:
https://google.github.io/styleguide/cppguide.html#Function_Names. Specifically, ordinary methods
are named like {{Foo::BarBaz()}}, but "accessor" methods for get / set are named {{Foo::bar()}}.
That is why {{RegionRequest::actions()}} is fine. Let's follow these conventions going forward.

- We should not be doing dynamic cast to Get like this here: 
{code}
+    if (auto pget = dynamic_cast<Get *>(action)) {
+      auto pb_get = RequestConverter::ToGet(*pget);
{code} 
The reason is that when we add Put objects as actions, then we have to use {{typeid}} and
will then have to depend on RTTI. Maybe we should templetize the {{Action}} class instead
of using Row? Anyway, let's leave this for a follow up issue, because I do not want to hold
on this patch for longer. 
-  Why is this returning a Future? You should just return {{std::unique_ptr<Request>}}.

{code}
+folly::Future<std::unique_ptr<Request>> RequestConverter::ToMultiRequest(
{code}
- remove this: 
{code}
+  // return folly::makeFuture<std::unique_ptr<hbase::MultiResponse>>(std::move(multi_results));
{code}
- We are not gonna use region load statistics for the foreseeable future, so you can ignore
that for future patches. Feel free to keep these here since we already have them. 
{code}
+    hbase::pb::MultiRegionLoadStats stats = multi_resp->regionstatistics();
{code}
- Here you are converting a unique_ptr from the factory to a shared_ptr: 
{code}
+        auto unique_result = ToResult(roe.result(), resp.cell_scanner());
+        result = std::make_shared<Result>(*unique_result);
{code}
The efficient way to do this is something like: 
{code}
+        auto unique_result = ToResult(roe.result(), resp.cell_scanner());
+        result = std::move(unique_result);
{code}
- {{ResponseType}} is not used, remove. 
- {{+  explicit MultiResponse(const std::string& region_name);}} does not look correct
(multi response is for multiple regions). And the constructor does not seem to be used, remove
if so. 
- We had talked (offline) about the maps in {{RegionRequest}} and {{ServerRequest}} being
Concurrent maps. It is pretty important to have these data structures to be thread-safe. Remember
that in the multi-request scatter / gather logic, the Batch caller will be scheduling RPCs
to multiple servers, and each action maybe retried by the retry timer. That is why we have
to have some kind of thread-safety in these. You can either use Concurent maps from folly,
or, use mutexes for accessing these maps.  




> [C++] Classes required for implementation of BatchCallerBuilder
> ---------------------------------------------------------------
>
>                 Key: HBASE-17771
>                 URL: https://issues.apache.org/jira/browse/HBASE-17771
>             Project: HBase
>          Issue Type: Sub-task
>            Reporter: Sudeep Sunthankar
>            Assignee: Sudeep Sunthankar
>             Fix For: HBASE-14850
>
>         Attachments: HBASE-17771.HBASE-14850.v1.patch, HBASE-17771.HBASE-14850.v2.patch
>
>
> Separating depedencies of BatchCallerBuilder.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

Mime
View raw message