Dan Burkert has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11018 )

Change subject: hms-tool: refactor check tool and combine upgrade and fix
......................................................................


Patch Set 11:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/11018/10/src/kudu/tools/tool_action_hms.cc@169
PS10, Line 169: PrintTables
> It looks like PrintKuduTables can also be embedded into this function?
Not entirely sure I follow, but PrintKuduTables can't call into PrintTables 
without a change in behavior, because PrintTables shows a different set of 
columns.


http://gerrit.cloudera.org:8080/#/c/11018/10/src/kudu/tools/tool_action_hms.cc@366
PS10, Line 366: Suggestion: rename the Kudu table(s) to be Hive-compatible,
> Should we call out this needs to be run before running the fix tool. I am g
Done


http://gerrit.cloudera.org:8080/#/c/11018/10/src/kudu/tools/tool_action_hms.cc@415
PS10, Line 415:  endl
              :           << "Sug
> nit: this can be put upfront L408 to avoid dup code.
It's somewhat complicated to do that because of the colon in the first case:

    cout << endl
         << "Suggestion: use the fix tool to repair the Kudu and Hive Metastore 
catalog metadata";
    if (report.orphan_hms_tables.empty()) {
      cout << ":" << endl << "\t$ kudu hms fix " << master_addrs << endl;
    } else {
      cout << endl
           << "and drop Hive Metastore table(s) which reference non-existent 
Kudu tables:" << endl
           << "\t$ kudu hms fix --drop_orphan_hms_tables " << master_addrs << 
endl;
    }

I think it reads better as is, but I can switch it if you feel strongly.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieee478002eb56a278cfd74255670baaf28109b8f
Gerrit-Change-Number: 11018
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Dan Burkert <[email protected]>
Gerrit-Reviewer: Hao Hao <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Mon, 30 Jul 2018 17:34:21 +0000
Gerrit-HasComments: Yes

Reply via email to