Alexey Serbin 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? Rephrased in an attempt to clarify on that. 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? Sure, we can do that: the very first version of this patch had this enabled. For some reason, after processing feedback from PS1 I was under impression we'd like to report on that only if requested explicitly. By after re-reading your comments I think we can enable this by default because 1) It could help spotting a misconfiguration 2) Warning don't lead to non-zero exit status anyways, so it's safe from the scripting/API compatibility point 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? Using C-string constants would not allow for using those in the description of flags, unfortunately. 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 i Replaced with strings::Split(). Not sure I understand the 'embed it directly in the loop' part. 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 Indeed. 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? Nope, set_symmetric_difference needs elements to be ordered to operate correctly: https://en.cppreference.com/w/cpp/algorithm/set_symmetric_difference Another option might be using std::vector, and then calling sort() && unique() before supplying to set_symmetric_difference(). That might bring a bit of performance gain in case larger flag categories. Is it worth it? 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? Done 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 vers Done -- 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 19:13:35 +0000 Gerrit-HasComments: Yes
