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