Bankim Bhavsar 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: (7 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] 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 in the tool itself before invoking the RPC to the master/tserver, right? What happens currently without this change? 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 // 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. Because you can't really proceed with "<unknown_host>". 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. Also it'd be good to capture the status explicitly so that we can print the error in case the assertion is negative. string cmd = Substitute("cluster ksck $0,$0", master_addr); Status s = RunActionStderrString(cmd, &out); ASSERT_TRUE(s.IsRuntimeError()) << s.ToString(); 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@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 certain about success and then move the local master addresses to out parameter. It's better to use the for range loop. for (const auto& master : master_addresses) No need to populate the duplicate_masters string unless duplicates are detected which can be concatenated using strings:Join function. 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. -- 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: 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: Mon, 01 Mar 2021 17:46:42 +0000 Gerrit-HasComments: Yes
