Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10217 )
Change subject: KUDU-2191: HMS Metadata Consistency Check Tool ...................................................................... Patch Set 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/10217/11/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/10217/11/src/kudu/tools/kudu-tool-test.cc@2188 PS11, Line 2188: // 2. Create a table in Kudu and in the HMS with the same table name Should this cover the case of master address not matching? http://gerrit.cloudera.org:8080/#/c/10217/11/src/kudu/tools/kudu-tool-test.cc@2195 PS11, Line 2195: ASSERT_OK(CreateKuduTable(kudu_client, "default.b")); It's pretty difficult to see what each of the a/b/c/d/e tables are testing specifically. I'd suggest moving the comment from the block to right before the initialization of each, or perhaps name them to indicate the case. http://gerrit.cloudera.org:8080/#/c/10217/10/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10217/10/src/kudu/tools/tool_action_hms.cc@394 PS10, Line 394: } > No, it doesn't. I was thinking add it here so later the fix tool can take c I think it's still worth warning about legacy tables, otherwise the admin might not know to run the upgrade tool? Perhaps you could just output a list of legacy tables, and hint that the upgrade tool should be used. http://gerrit.cloudera.org:8080/#/c/10217/11/src/kudu/tools/tool_action_hms.cc File src/kudu/tools/tool_action_hms.cc: http://gerrit.cloudera.org:8080/#/c/10217/11/src/kudu/tools/tool_action_hms.cc@368 PS11, Line 368: if (!s.ok() && !s.IsNotFound()) { Might be worth seeing if you can remove some of the nested if/else levels here. Perhaps: if (s.ok() && !isSynced(master_addresses_str, kudu_table, hms_table)) { ... } else if (s.IsNotFound()) { ... } else { // All errors other than IsNotFound are fatal. RETURN_NOT_OK(s); } http://gerrit.cloudera.org:8080/#/c/10217/11/src/kudu/tools/tool_action_hms.cc@396 PS11, Line 396: // 3. If any Kudu tables are not present in the HMS, consider it as an out of sync Sorry my suggestion before on R10 was bad. This should be either If any Kudu tables are not present in the HMS, consider _them as out of sync tables_. or If any Kudu _table is not present_ in the HMS, consider it as an out of sync table. -- To view, visit http://gerrit.cloudera.org:8080/10217 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3495e9c755571b85beaecca849f83457b592ee90 Gerrit-Change-Number: 10217 Gerrit-PatchSet: 11 Gerrit-Owner: Hao Hao <hao....@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: Wed, 20 Jun 2018 17:40:59 +0000 Gerrit-HasComments: Yes
