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 Mon, 13 Mar 2017 21:55:41 GMT

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

Enis Soztutar commented on HBASE-17771:

- Changes in {{core/BUCK}} undos the previous changes from HBASE-17728. This is probably due
to rebasing, but we need HBASE-17728. 
- {{Action::OriginalIndex()}} should be named {{Action::original_index()}} and {{Action::GetAction()}}
should be {{Action::action()}}. Maybe in the other classes as well (my reading of the conventions
is that if the class method is an accessor to a class field, you can name it like foo_bar(),
instead of FooBar(). 
- Class {{MultiAction}} is not needed?. Seems to be coming from the AsyncProcess based impl.
I've checked the patch at HBASE-17576, not used there as well. 
- Remove commented out code: 
+  /*static std::shared_ptr<hbase::MultiResponse> GetResults(const std::shared_ptr<Request>&
+   std::unique_ptr<Response> resp);*/
The non-commented out version takes Request by raw-pointer which is probably not what we want.

- The CellScanner::Advance() might return false as well for the below code: 
-    while (cell_scanner->Advance()) {
+    int num_cells = 0;
+    while (num_cells != result.associated_cell_count()) {
+      cell_scanner->Advance();
So, I think you should check for that, and throw an exception if we wanted to read that many
cells, but the CellScanner::Advance() is returning false.
- Are you gonna address this? {{+// TODO Revisit this class once}} 
- RegionRequest::ActionList is a shared_ptr, while ServerRequest::ActionsByRegion is not.
I think you don't need the shared_ptr in RegionRequest::ActionList, especially because you
are returning via {{const &}}. 
- RequestConverter::ToGet() is good, but it is duplicating the code. Maybe make it so that
ToGetRequest() uses that. 

> [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
> Separating depedencies of BatchCallerBuilder.

This message was sent by Atlassian JIRA

View raw message