[kudu-CR] KUDU-2884: [hms] Improve master address matching
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Though the default port is considered when matching master addresses, it is not included in the automated hms tool tests given the tests use random ephemeral ports. Instead a test for MasterAddressesToSet and UnorderedHostPortSet equality was added. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Reviewed-on: http://gerrit.cloudera.org:8080/16420 Reviewed-by: Andrew Wong Reviewed-by: Alexey Serbin Tested-by: Kudu Jenkins --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 5 files changed, 134 insertions(+), 34 deletions(-) Approvals: Andrew Wong: Looks good to me, approved Alexey Serbin: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 7: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@4103 PS3, Line 4103: LOG(INFO) << "Modified Masters: " << JoinStrings(modified_addrs, ","); > Given this is a test, I figured it was fine to leave and may help debug any I'm just under impression that many of our tests are too chatty :) But if you see value in this extra information printed, it makes sense to keep it, sure. -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 09 Sep 2020 18:35:59 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 09 Sep 2020 18:35:35 + Gerrit-HasComments: No
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@577 PS3, Line 577: _vector)); : *res = UnorderedHostPortSet(hp_vector.begin(), hp_vector.end()); : > Seems like this code was changed since we're emplacing directly into 'res' Done http://gerrit.cloudera.org:8080/#/c/16420/6/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/16420/6/src/kudu/tools/tool_action_common.cc@578 PS6, Line 578: *res = UnorderedHostPortSet(hp_vector.begin(), hp_vector.end()); : : / > Nit: res->insert(hp_vector.begin(), hp_vector.end()) Done -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 09 Sep 2020 18:24:40 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16420 to look at the new patch set (#7). Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Though the default port is considered when matching master addresses, it is not included in the automated hms tool tests given the tests use random ephemeral ports. Instead a test for MasterAddressesToSet and UnorderedHostPortSet equality was added. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 5 files changed, 134 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/16420/7 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/16420 )
Change subject: KUDU-2884: [hms] Improve master address matching
..
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/16420/6/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/16420/6/src/kudu/tools/tool_action_common.cc@578
PS6, Line 578: for (auto hp : hp_vector) {
: res->emplace(std::move(hp));
: }
Nit: res->insert(hp_vector.begin(), hp_vector.end())
--
To view, visit http://gerrit.cloudera.org:8080/16420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7
Gerrit-Change-Number: 16420
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 09 Sep 2020 18:19:11 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Andrew Wong has posted comments on this change. (
http://gerrit.cloudera.org:8080/16420 )
Change subject: KUDU-2884: [hms] Improve master address matching
..
Patch Set 6: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@3991
PS3, Line 3991: vector master_addrs;
: for (const auto& hp : cluster_->master_rpc_addrs()) {
: master_addrs.emplace_back(hp.ToString());
: }
> Nit: Can use std::transform instead.
I'm kind of on the fence about replacing simple for loops with std::transform,
std::for_each, etc. While more succinct, it's an extra step of mental
gymnastics to reason about it with the lambda and all, especially if reading
this code coming from another language.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@577
PS3, Line 577: _vector));
: for (auto hp : hp_vector) {
:
> Done
Seems like this code was changed since we're emplacing directly into 'res' in
PS6.
Consider constructing 'res' directly once we know we'll return success, e.g.
RETURN_NOT_OK(ParseStrings(...));
*res = UnorderedHostPortSet(hp_vector.begin(), hp_vector.end());
This also has the sometimes nice property that if there's an error, we don't
update 'res'. Though I don't feel strongly about it.
--
To view, visit http://gerrit.cloudera.org:8080/16420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7
Gerrit-Change-Number: 16420
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 09 Sep 2020 18:03:21 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16420 to look at the new patch set (#6). Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Though the default port is considered when matching master addresses, it is not included in the automated hms tool tests given the tests use random ephemeral ports. Instead a test for MasterAddressesToSet and UnorderedHostPortSet equality was added. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 5 files changed, 136 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/16420/6 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16420 to look at the new patch set (#5). Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Though the default port is considered when matching master addresses, it is not included in the automated hms tool tests given the tests use random ephemeral ports. Instead a test for MasterAddressesToSet and UnorderedHostPortSet equality was added. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 5 files changed, 135 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/16420/5 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/16420 )
Change subject: KUDU-2884: [hms] Improve master address matching
..
Patch Set 3:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@3972
PS3, Line 3972:
> Alignment.
Done
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@3972
PS3, Line 3972:
> Alignment.
Done
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@4091
PS3, Line 4091: // Test case: orphan table in the HMS with different, but
functionally
: // the same master addresses.
> Do you mind adding a 'negative' case here as well, so the set of master add
I added this to my more pointed TestMasterAddressesToSet along with a default
port handling test.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@4103
PS3, Line 4103: LOG(INFO) << "Modified Masters: " <<
JoinStrings(modified_addrs, ",");
> If this was added just for debugging purposes, maybe it's possible to drop
Given this is a test, I figured it was fine to leave and may help debug any
failures in the future.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.h@30
PS3, Line 30:
> Nit: double quotes instead of <> brackets
Done
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@570
PS3, Line 570: _arg
> Nit: _arg in the parameter name seems unnecessary.
It just matches the others above. I could change it for _str, but I am not sure
it matters. Having it just be master_addresses isn't great because most places
in the code base that is used for a vector or addresses.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@571
PS3, Line 571: UnorderedHostPortSet* res
> I'm not sure I understand how the 'res' parameter is used in this function
No, I had a local change that wasn't pushed. It's fixed in the next revision.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@571
PS3, Line 571: UnorderedHostPortSet* res) {
> Alignment
Done
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@577
PS3, Line 577: for (auto hp : hp_vector) {
: hp_set.emplace(std::move(hp));
: }
> Can simply specify vector in the constructor of the set.
Done
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@583
PS3, Line 583: vector addr_list;
: for (const auto& addr : hp_set) {
: addr_list.emplace_back(addr);
: }
> Same here
Done
--
To view, visit http://gerrit.cloudera.org:8080/16420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7
Gerrit-Change-Number: 16420
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 09 Sep 2020 13:05:25 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Hello Alexey Serbin, Kudu Jenkins, Andrew Wong, Bankim Bhavsar, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16420 to look at the new patch set (#4). Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Though the default port is considered when matching master addresses, it is not included in the automated hms tool tests given the tests use random ephemeral ports. Instead a test for MasterAddressesToSet and UnorderedHostPortSet equality was added. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 5 files changed, 133 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/16420/4 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@4091 PS3, Line 4091: // Test case: orphan table in the HMS with different, but functionally : // the same master addresses. Do you mind adding a 'negative' case here as well, so the set of master addresses would contain addresses that differ at * actual addresses, but still having the same port numbers (or a combination of omitted port numbers) * actual port numbers, but maybe still having same hostnames ? http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@4103 PS3, Line 4103: LOG(INFO) << "Modified Masters: " << JoinStrings(modified_addrs, ","); If this was added just for debugging purposes, maybe it's possible to drop this now when the test is ready? http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@571 PS3, Line 571: UnorderedHostPortSet* res I'm not sure I understand how the 'res' parameter is used in this function aside from calling 'res->clear()' at line 572. Do I miss something? -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 09 Sep 2020 01:55:01 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Bankim Bhavsar has posted comments on this change. (
http://gerrit.cloudera.org:8080/16420 )
Change subject: KUDU-2884: [hms] Improve master address matching
..
Patch Set 3: Code-Review+1
(7 comments)
Minor issues.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@3972
PS3, Line 3972:
Alignment.
Sidebar: I know CLion tends to do this for macros like RETURN_NOT_OK(). Need to
figure why it does so.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/kudu-tool-test.cc@3991
PS3, Line 3991: vector master_addrs;
: for (const auto& hp : cluster_->master_rpc_addrs()) {
: master_addrs.emplace_back(hp.ToString());
: }
Nit: Can use std::transform instead.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.h
File src/kudu/tools/tool_action_common.h:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.h@30
PS3, Line 30:
Nit: double quotes instead of <> brackets
Another thing to teach CLion :)
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@570
PS3, Line 570: _arg
Nit: _arg in the parameter name seems unnecessary.
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@571
PS3, Line 571: UnorderedHostPortSet* res) {
Alignment
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@577
PS3, Line 577: for (auto hp : hp_vector) {
: hp_set.emplace(std::move(hp));
: }
Can simply specify vector in the constructor of the set.
UnorderedHostPortSet hp_set(hp_vector.begin(), hp_vector.end());
http://gerrit.cloudera.org:8080/#/c/16420/3/src/kudu/tools/tool_action_common.cc@583
PS3, Line 583: vector addr_list;
: for (const auto& addr : hp_set) {
: addr_list.emplace_back(addr);
: }
Same here
--
To view, visit http://gerrit.cloudera.org:8080/16420
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7
Gerrit-Change-Number: 16420
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Bankim Bhavsar
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 08 Sep 2020 17:47:54 +
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 3: Verified+1 Code-Review+1 (1 comment) http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc@4085 PS1, Line 4085: > It's a little difficult to do this given the tests need to run on random ep Ah ok. Could you mention that in the commit message and maybe manually test on a real cluster? -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 08 Sep 2020 16:55:45 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 3: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 08 Sep 2020 16:40:52 + Gerrit-HasComments: No
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Grant Henke has removed a vote on this change. Change subject: KUDU-2884: [hms] Improve master address matching .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc@4085 PS1, Line 4085: // Test case: orphan table in the HMS with different, but functionally > Could you also add a test for the port-ignoring behavior? It's a little difficult to do this given the tests need to run on random ephemeral ports for each node. I could try to plumb through some test only ability to set/change the default value to the current ports being used I wasn't sure it was worth it. http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc@4090 PS1, Line 4090: Reverse > nit: this doesn't seem to be reversing the order. Done http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/tool_action_common.h@191 PS1, Line 191: Status MasterAddressesToSet( > nit: can you add docs, including the fact that this adds a default port? Done http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/tool_action_common.h@192 PS1, Line 192: string > nit: const ref to avoid a copy Done -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 08 Sep 2020 15:30:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Hello Kudu Jenkins, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16420 to look at the new patch set (#3). Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 4 files changed, 102 insertions(+), 33 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/16420/3 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16420 ) Change subject: KUDU-2884: [hms] Improve master address matching .. Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc@4085 PS1, Line 4085: // Test case: orphan table in the HMS with different, but functionally Could you also add a test for the port-ignoring behavior? http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/kudu-tool-test.cc@4090 PS1, Line 4090: Reverse nit: this doesn't seem to be reversing the order. http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/tool_action_common.h File src/kudu/tools/tool_action_common.h: http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/tool_action_common.h@191 PS1, Line 191: Status MasterAddressesToSet( nit: can you add docs, including the fact that this adds a default port? http://gerrit.cloudera.org:8080/#/c/16420/1/src/kudu/tools/tool_action_common.h@192 PS1, Line 192: string nit: const ref to avoid a copy -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 04 Sep 2020 20:21:27 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/16420 to look at the new patch set (#2). Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 4 files changed, 73 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/16420/2 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] KUDU-2884: [hms] Improve master address matching
Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/16420 Change subject: KUDU-2884: [hms] Improve master address matching .. KUDU-2884: [hms] Improve master address matching This patch improves the master address matching in the `kudu hms fix` tool. Instead of expecting a literal match of the address strings, it expects the match to be functionally the same. This means that the addresses can be missing the port (using the default), duplicated, and re-ordered. The HMS will still consider them the same if they match the clusters masters functionally. Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 --- M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_hms.cc 4 files changed, 71 insertions(+), 16 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/20/16420/1 -- To view, visit http://gerrit.cloudera.org:8080/16420 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I00059ff1b5134f7e5d6ea8be4fdf0701aa56b0c7 Gerrit-Change-Number: 16420 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke
