Hao Hao has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10075 )

Change subject: KUDU-2191: Metadata Upgrade Tool
......................................................................


Patch Set 13:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc
File src/kudu/hms/hms_catalog-test.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@237
PS12, Line 237:     table.__set_parameters({
> I think it would be better to remove this behavior from the KuduMetastorePl
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog-test.cc@456
PS12, Line 456: hms_tables_map, kExternalTableName));
> instead of calling GetTable and passing in the parameter, perhaps it's bett
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h
File src/kudu/hms/hms_catalog.h:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@91
PS12, Line 91:                                   const std::string& db_name,
> This is the name of the table as stored in the HMS, right?  If so, it might
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@96
PS12, Line 96: or a
> nit: 'is the legacy...'
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.h@100
PS12, Line 100:   // This method will fail if the HMS is unreachable.
> I'm not quite following this bit - why is storage_handler taken as an argum
In the following patch, the same method is used to retrieve non-legacy tables. 
So it takes storage_handler as an argument. The reason why the need to have a 
map is to avoid O(n) table lookup given a Kudu table's name.


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc
File src/kudu/hms/hms_catalog.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/hms/hms_catalog.cc@193
PS12, Line 193:
> nit: 'non-legacy'
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto
File src/kudu/tools/tool.proto:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool.proto@43
PS12, Line 43:   // Whether or not to create a Hive Metastore, and/or enable 
Kudu Hive
             :   // Metastore integration.
             :   optional HmsMode hms_mode = 7;
             :
             :   // The directory where the cluster's data and l
> Same feedback.
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc
File src/kudu/tools/tool_action_hms.cc:

http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@67
PS12, Line 67: non-Impala
> 'non-Impala' here and below (non should usually be hyphen separated from th
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@136
PS12, Line 136: 
> This should probably go before creating the HmsCatalog.
Done


http://gerrit.cloudera.org:8080/#/c/10075/12/src/kudu/tools/tool_action_hms.cc@145
PS12, Line 145:       .master_server_addrs(master_addresses)
> Maybe add a comment here?  I'm not sure why this would need to have non-def
Oops, thanks for pointing it out. I think actually it is not needed.



--
To view, visit http://gerrit.cloudera.org:8080/10075
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1c8488e1c7b04b8bedc31c83b1496b53bae49cb3
Gerrit-Change-Number: 10075
Gerrit-PatchSet: 13
Gerrit-Owner: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: Hao Hao <hao....@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Fri, 11 May 2018 01:50:06 +0000
Gerrit-HasComments: Yes

Reply via email to