[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/17141 )
Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster
ksck
..
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@657
PS5, Line 657: if (master.empty())
: continue;
Nit: I'm not sure whether it's part of style guide but for "if" if there is a
single stmt and without any braces then the stmt should be on the same single
line.
if (master.empty()) continue;
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@660
PS5, Line 660: er)+"
> That said, maybe instead keep an unordered_set of duplicated masters and
> output that via JoinStrings()? (see gutil/strings/join.h)
+1 to above.
http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@665
PS5, Line 665: if (!duplicate_masters.empty())
: return Status::InvalidArgument("Duplicate master addresses
specified: " + duplicate_masters);
: *master_addresses = std::move(master_addresses_local);
Same as above about either using braces {} for "if" if the stmt is on next line.
Else it can cause confusion whether the L667 is part of the if stmt or no.
--
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: 5
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 10 Mar 2021 21:54:54 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17141 ) Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. Patch Set 5: (7 comments) Overall looks good, just more style nits. 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 the tool > This was the Jira number and the Subject of the Jira. I will include more c nit: actually this meant something like [tool] KUDU-3226: Validate list of masters for ksck Look through https://github.com/apache/kudu/commits/master for more examples http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/kudu-tool-test.cc@6103 PS5, Line 6103: // nit: here and elsewhere with comments, add a space after the //, as is used throughout the codebase http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/kudu-tool-test.cc@6107 PS5, Line 6107: ()) nit: it's probably a good habit to build to start using curly braces for these code blocks, even though they're not strictly necessary for one-liners. The GSG allows this for historical reasons, but with some caveats https://google.github.io/styleguide/cppguide.html#Conditionals 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:: > Done I don't see this update in PS5? http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@653 PS5, Line 653: std:: nit: don't need the std:: here either. http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@654 PS5, Line 654: /Loop through the master addresses to find the duplicate. If there is no master specified : //d nit: same here with regards to adding a space after //. http://gerrit.cloudera.org:8080/#/c/17141/5/src/kudu/tools/tool_action_common.cc@660 PS5, Line 660: er)+" nit: I don't think it's strictly requested by the GSG, but for the most part, we typically add spaces between operators. I.e. duplicate_masters.append(master) + " "; That said, maybe instead keep an unordered_set of duplicated masters and output that via JoinStrings()? (see gutil/strings/join.h) -- 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: 5 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 10 Mar 2021 01:08:21 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17141 ) Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@9 PS2, Line 9: kudu cluster : ksck/rebalance > That is correct. But I manually tried all the commands which have masters t I stand corrected. I tested with the new binary and here are all the modes which are affected (detect duplicate master addresses and report) by this change : kudu cluster kudu perf kudu table kudu master kudu tablet kudu server The existing binary only kudu cluster mode reports an issue with duplicate master addresses which is because the rebalancer calls ksck internally[1] and ksck constructs a map of kudu masters' UUIDs and finds duplicates [2] and dies. Rest of the modes do not construct a map of the master UUIDs and hence do not die. [1] https://github.com/apache/kudu/blob/master/src/kudu/tools/rebalancer_tool.cc#L626-L635 [2] https://github.com/apache/kudu/blob/master/src/kudu/tools/ksck.cc#L339-L344 -- 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: 5 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 09 Mar 2021 04:13:24 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17141 to look at the new patch set (#5). Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. KUDU-3226 Validate List of Masters for the tool kudu cluster ksck This patch checks for duplicate kudu masters provided in kudu cluster ksck/rebalance commands. It only does a string comparison check to detect and report all the duplicate master(/s). If a user provides the IP address and hostname of the same master, it won't be able to catch the duplicates. That seems more of a deliberate attempt rather than a common mistake a user could make and hence not considering that case. Also wrote a simple test case to test this functionality. Decided not to spin up a test cluster for this as this is more of client side check and a master is not actually contacted. This change got conflicted with another test i.e. AdminCliTest.TestAuthzResetCacheIncorrectMasterAddressList. Updated this test as well to by creating a new external mini cluster with three masters and running the command in the test with just one master Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc 3 files changed, 41 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/17141/5 -- 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: newpatchset Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f Gerrit-Change-Number: 17141 Gerrit-PatchSet: 5 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17141 to look at the new patch set (#4). Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. KUDU-3226 Validate List of Masters for the tool kudu cluster ksck This patch checks for duplicate kudu masters provided in kudu cluster ksck/rebalance commands. It only does a string comparison check to detect and report all the duplicate master(/s). If a user provides the IP address and hostname of the same master, it won't be able to catch the duplicates. That seems more of a deliberate attempt rather than a common mistake a user could make and hence not considering that case. Also wrote a simple test case to test this functionality. Decided not to spin up a test cluster for this as this is more of client side check and a master is not actually contacted. This change got conflicted with another test i.e. AdminCliTest.TestAuthzResetCacheIncorrectMasterAddressList. Updated this test as well to by creating a new external mini cluster with three masters and running the command in the test with just one master Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc 3 files changed, 41 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/17141/4 -- 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: newpatchset Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f Gerrit-Change-Number: 17141 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/17141 ) Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@9 PS2, Line 9: provided > specified Done http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@10 PS2, Line 10: comparison > comparison Done http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@9 PS2, Line 9: kudu cluster : ksck/rebalance > I guess that's true not only for these, but other commands where the list o That is correct. But I manually tried all the commands which have masters to be specified and appears it is just the ksck/rebalance are the ones calling PaserMasterAddresses(). http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@21 PS2, Line 21: Updated this : test as well > It seems that change isn't included into PS2: Patch set 2 did not have any changes, my bad. Patch set 3 is the one which is supposed to have all the changes done. Just pushed it http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@654 PS2, Line 654: //Loop through the master addresses to find the duplicate. If there is no master specified > style nit: add a space after // Done http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@656 PS2, Line 656: for (const auto& master : master_addresses_local > You could use Done http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@657 PS2, Line 657: master.empty()) > Is an empty master address allowed at all? No it isn't, that is caught in the later in the code flow. Just did not want to report/detect empty masters as duplicate here http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@667 PS2, Line 667: retu > nit: don't use 'else' after a return; see https://llvm.org/docs/CodingStand 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: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 08 Mar 2021 22:41:21 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Hello Alexey Serbin, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17141 to look at the new patch set (#3). Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. KUDU-3226 Validate List of Masters for the tool kudu cluster ksck This patch checks for duplicate kudu masters provided in kudu cluster ksck/rebalance commands. It only does a string comparison check to detect and report all the duplicate master(/s). If a user provides the IP address and hostname of the same master, it won't be able to catch the duplicates. That seems more of a deliberate attempt rather than a common mistake a user could make and hence not considering that case. Also wrote a simple test case to test this functionality. Decided not to spin up a test cluster for this as this is more of client side check and a master is not actually contacted. This change got conflicted with another test i.e. AdminCliTest.TestAuthzResetCacheIncorrectMasterAddressList. Updated this test as well to by creating a new external mini cluster with three masters and running the command in the test with just one master Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc 3 files changed, 40 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/17141/3 -- 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: newpatchset Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f Gerrit-Change-Number: 17141 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Abhishek Chennaka has abandoned this change. ( http://gerrit.cloudera.org:8080/17149 ) Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. Abandoned Duplicated accidentally -- To view, visit http://gerrit.cloudera.org:8080/17149 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Iaeaa3fc455b3484ff1a5f40e94c0071cd638b47c Gerrit-Change-Number: 17149 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/17141 )
Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster
ksck
..
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@9
PS2, Line 9: reported
specified
?
http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@10
PS2, Line 10: comparision
comparison
http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@9
PS2, Line 9: kudu cluster
: ksck/rebalance
I guess that's true not only for these, but other commands where the list of
master addresses is parsed using ParseMasterAddresses() ?
http://gerrit.cloudera.org:8080/#/c/17141/2//COMMIT_MSG@21
PS2, Line 21: Updated this
: test as well
It seems that change isn't included into PS2:
/home/jenkins-slave/workspace/kudu-master/1/src/kudu/tools/kudu-admin-test.cc:2260
Value of: err
Expected: has substring "Invalid argument: list of master addresses provided
(127.0.185.190:44921,127.0.185.190:44921) does not match the actual cluster
configuration (127.0.185.190:44921)"
http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@654
PS2, Line 654: //Loop through the master addresses to find the duplicate. If
there is no master specified
style nit: add a space after //
http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@656
PS2, Line 656: for (int i = 0; isize(); i++)
You could use
for (const auto& address : *master_addresses) {
}
instead of accessing the element by index.
http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@657
PS2, Line 657: (master_addresses->at(i)).empty()
Is an empty master address allowed at all?
http://gerrit.cloudera.org:8080/#/c/17141/2/src/kudu/tools/tool_action_common.cc@667
PS2, Line 667: else
nit: don't use 'else' after a return; see
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
--
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: 2
Gerrit-Owner: Abhishek Chennaka
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 07 Mar 2021 18:30:45 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Abhishek Chennaka has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17149 Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. KUDU-3226 Validate List of Masters for the tool kudu cluster ksck This patch checks for duplicate kudu masters reported in kudu cluster ksck/rebalance commands. It only does a string comparision check to detect and report all the duplicate master(/s). If a user provides the IP address and hostname of the same master, it won't be able to catch the duplicates. That seems more of a deliberate attempt rather than a common mistake a user could make and hence not considering that case. Also wrote a simple test case to test this functionality. Decided not to spin up a test cluster for this as this is more of client side check and a master is not actually contacted. This change got conflicted with another test i.e. AdminCliTest.TestAuthzResetCacheIncorrectMasterAddressList. Updated this test as well to by creating a new external mini cluster with three masters and running the command in the test with just one master Change-Id: Iaeaa3fc455b3484ff1a5f40e94c0071cd638b47c --- M src/kudu/tools/kudu-admin-test.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc 3 files changed, 24 insertions(+), 20 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/17149/1 -- To view, visit http://gerrit.cloudera.org:8080/17149 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iaeaa3fc455b3484ff1a5f40e94c0071cd638b47c Gerrit-Change-Number: 17149 Gerrit-PatchSet: 1 Gerrit-Owner: Abhishek Chennaka
[kudu-CR] KUDU-3226 Validate List of Masters for the tool kudu cluster ksck
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/17141 to look at the new patch set (#2). Change subject: KUDU-3226 Validate List of Masters for the tool kudu cluster ksck .. KUDU-3226 Validate List of Masters for the tool kudu cluster ksck This patch checks for duplicate kudu masters reported in kudu cluster ksck/rebalance commands. It only does a string comparision check to detect and report all the duplicate master(/s). If a user provides the IP address and hostname of the same master, it won't be able to catch the duplicates. That seems more of a deliberate attempt rather than a common mistake a user could make and hence not considering that case. Also wrote a simple test case to test this functionality. Decided not to spin up a test cluster for this as this is more of client side check and a master is not actually contacted. This change got conflicted with another test i.e. AdminCliTest.TestAuthzResetCacheIncorrectMasterAddressList. Updated this test as well to by creating a new external mini cluster with three masters and running the command in the test with just one master Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc 2 files changed, 28 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/17141/2 -- 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: newpatchset Gerrit-Change-Id: I2f3b2b7dcf2ac78cb95cf43242651e3ce8fddf6f Gerrit-Change-Number: 17141 Gerrit-PatchSet: 2 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
