Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15298 )
Change subject: [ksck] report on misconfiguration for flag categories ...................................................................... Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG@12 PS2, Line 12: differences between masters' and tablet servers' settings : for same flags. Isn't this redundant w.r.t. the first half of the sentence? http://gerrit.cloudera.org:8080/#/c/15298/2//COMMIT_MSG@21 PS2, Line 21: To use the newly introduced functionality, add : --flags_categories_to_check=<comma-separated-list-of-flags-categories> : to the 'kudu cluster ksck' invocation. Thoughts on enabling this by default? http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc File src/kudu/tools/ksck.cc: http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@57 PS2, Line 57: #define STR_FLAGS_CATEGORY_TIME_SOURCE "time_source" : #define STR_FLAGS_CATEGORY_UNUSUAL "unusual" Could use C-string constants? http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@245 PS2, Line 245: SplitStringUsing(str, ",", &categories_str); I think we typically using strings::Split rather than this. You can embed it directly in the loop. http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@390 PS2, Line 390: void Ksck::AddFlagsToFlagMap(const GetFlagsResponsePB& flags, : const string& server_address, : KsckFlagToServersMap* flags_to_servers_map) { : CHECK(flags_to_servers_map); : for (const auto& f : flags.flags()) { : const pair<string, string> key(f.name(), f.value()); : if (!InsertIfNotPresent(flags_to_servers_map, key, { server_address })) { : FindOrDieNoPrint(*flags_to_servers_map, key).push_back(server_address); : } : } : } Seems pretty straight-forward to reuse the existing AddFlagsToFlagMaps with a null flag_tags_map? http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@722 PS2, Line 722: set<KsckFlag> masters_flags; Could use unordered_sets here? http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck.cc@753 PS2, Line 753: // The flag/value pair must be either of masters' or tservers'. : DCHECK(false); LOG(DFATAL) instead? http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck_results.h File src/kudu/tools/ksck_results.h: http://gerrit.cloudera.org:8080/#/c/15298/2/src/kudu/tools/ksck_results.h@174 PS2, Line 174: // Print a formatted summary of the flags in 'flag_to_servers_map', : // indicating which servers have which (flag, value) pairs set. Could you reword this in terms of the existing PrintFlagTable, or vice versa? Would also be nice to avoid the overload; could you rename one? -- To view, visit http://gerrit.cloudera.org:8080/15298 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2afbfac9327f85b212ecf8d8f43a2139f90db6bb Gerrit-Change-Number: 15298 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Tue, 10 Mar 2020 07:06:27 +0000 Gerrit-HasComments: Yes
