Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17141 )
Change subject: KUDU-3226 Validate List of Masters for kudu ksck ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/17141/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17141/1//COMMIT_MSG@7 PS1, Line 7: KUDU-3226 Validate List of Masters for kudu ksck > To provide context, we also include the component. In this case [tool] This was the Jira number and the Subject of the Jira. I will include more context in it http://gerrit.cloudera.org:8080/#/c/17141/1//COMMIT_MSG@9 PS1, Line 9: This patch checks for duplicate kudu masters reported in kudu cluster : ksck/rebalance commands. > So this change is basically validating the input to ksck/rebalance command It throws the not so meaningful error message with stack trace: EG: F0111 03:11:14.265851 101976 map-util.h:410] Check failed: InsertIfNotPresent(collection, key, data) duplicate key: 98eea16246634b11bea207c6f397daa4 Check failure stack trace: *** @ 0x10d392d google::LogMessage::Fail() @ 0x10d5883 google::LogMessage::SendToLog() @ 0x10d3489 google::LogMessage::Flush() @ 0x10d620f google::LogMessageFatal::~LogMessageFatal() @ 0xc94216 InsertOrDie<>() @ 0xc8d59b kudu::tools::Ksck::CheckMasterConsensus() @ 0xc90583 kudu::tools::Ksck::Run() @ 0xc9100a kudu::tools::Ksck::RunAndPrintResults() @ 0xb44981 kudu::tools::(anonymous namespace)::RunKsck() @ 0xb46cb4 std::_Function_handler<>::_M_invoke() @ 0x1129ac2 kudu::tools::Action::Run() @ 0xbcc027 kudu::tools::DispatchCommand() @ 0xbcc9eb kudu::tools::RunTool() @ 0xae595d main @ 0x7fd165440505 __libc_start_main @ 0xb40ea1 (unknown) http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc@6103 PS1, Line 6103: //Execute a ksck giving the hostname of the local machine twice : //and it should report the duplicates found. > Nit: space after // Done http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc@6107 PS1, Line 6107: if (!GetHostname(&master_addr).ok()) : master_addr = "<unknown_host>"; > It'd be better to use ASSERT_OK() and fail the test here in that case. Beca In the test case ksck is just going to read the string and evaluate. So even the string "<unknown_host>" would be acceptable to run the test case in case the hostname is not returned. http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/kudu-tool-test.cc@6110 PS1, Line 6110: Substitute("cluster ksck $0,$0", master_addr) > Nit: For sake of readability it'd be good to put this in a separate line. Done http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@652 PS1, Line 652: std:: > nit: because of the 'using' declarations around L200, this can just be unor Done http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@659 PS1, Line 659: unique_masters.find(master_addresses->at(i)) != unique_masters.end() > nit: you can use the ContainsKey() function from gutil/map-util.h Suggested by Bankim as well on slack. Done http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@656 PS1, Line 656: for (int i = 0; i<master_addresses->size(); i++){ : if ((master_addresses->at(i)).empty()) : continue; : if (unique_masters.find(master_addresses->at(i)) != unique_masters.end()){ : duplicate_masters = duplicate_masters.append(master_addresses->at(i))+" "; : } : else : unique_masters.insert(master_addresses->at(i)); : } > It's best to not modify out parameter master_addresses unless we are certai Done http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@656 PS1, Line 656: for (int i = 0; i<master_addresses->size(); i++){ : if ((master_addresses->at(i)).empty()) : continue; : if (unique_masters.find(master_addresses->at(i)) != unique_masters.end()){ : duplicate_masters = duplicate_masters.append(master_addresses->at(i))+" "; : } : else : unique_masters.insert(master_addresses->at(i)); : } > +1 Done http://gerrit.cloudera.org:8080/#/c/17141/1/src/kudu/tools/tool_action_common.cc@667 PS1, Line 667: else > No else clause if there is a return above. Done -- To view, visit http://gerrit.cloudera.org:8080/17141 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f Gerrit-Change-Number: 17141 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 03 Mar 2021 21:34:41 +0000 Gerrit-HasComments: Yes
