Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10075 )
Change subject: KUDU-2191: Metadata Upgrade Tool ...................................................................... Patch Set 19: (7 comments) http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/kudu-tool-test.cc@2043 PS19, Line 2043: One with hive incompatible name. repeated just below http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@69 PS19, Line 69: DEFINE_bool(enable_input, false, I think true would be a better default here, since it's more annoying to have to set flags in interactive mode than in batch mode typically. http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@124 PS19, Line 124: Find an orphan table nit: 'Found orphan table $0.$1...' http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@159 PS19, Line 159: Table $0 already exists before upgrade I think something along the lines of 'Failed to upgrade legacy Impala table 'foo.bar' (Kudu table name: 'fuzz') because a Kudu table with name 'foo.bar' already exists' would be more clear http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@165 PS19, Line 165: auto create_hms_table = [&](const string& name) -> Status { I think this lambda is obscuring things a bit. For instance the SchemaFromKuduSchema call really only needs to happen once, and then after that it's a pretty simple function call. http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@170 PS19, Line 170: while (!s.ok() && FLAGS_enable_input && Would be good to have a test of this loop, just to have some coverage of the match, since it's brittle WRT Hive changing error messages. http://gerrit.cloudera.org:8080/#/c/10075/19/src/kudu/tools/tool_action_hms.cc@188 PS19, Line 188: return failures.front(); LOG(WARNING) the other errors so they get surfaced -- 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: 19 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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: Fri, 18 May 2018 18:51:15 +0000 Gerrit-HasComments: Yes
