You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/10/02 23:36:06 UTC
[kudu-CR] [mini cluster] fix accessing wildcard-bound NTP server
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14355
Change subject: [mini_cluster] fix accessing wildcard-bound NTP server
......................................................................
[mini_cluster] fix accessing wildcard-bound NTP server
In case if minicluster's test NTP server is bound to the wildcard
address, use the loopback address for communication with the server.
Prior to this patch, the ToolTest.TestRemoteReplicaCopy test scenario
would fail if running with built-in NTP client enabled.
Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
---
M src/kudu/mini-cluster/external_mini_cluster.cc
1 file changed, 7 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14355/1
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [mini chronyd] introduce MiniChronyd::address()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14355 )
Change subject: [mini_chronyd] introduce MiniChronyd::address()
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14355/2/src/kudu/clock/test/mini_chronyd.cc
File src/kudu/clock/test/mini_chronyd.cc:
http://gerrit.cloudera.org:8080/#/c/14355/2/src/kudu/clock/test/mini_chronyd.cc@176
PS2, Line 176: HostPort addr(options_.bindaddress == kWildcardIpAddr ? kLoopbackIpAddr
: : options_.bindaddress,
: options_.port);
: return addr;
Nit: could combine.
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 03 Oct 2019 06:27:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini cluster] fix accessing wildcard-bound NTP server
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14355 )
Change subject: [mini_cluster] fix accessing wildcard-bound NTP server
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14355/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/14355/1/src/kudu/mini-cluster/external_mini_cluster.cc@172
PS1, Line 172: ntp_endpoints.emplace_back(
Would it be clearer if MiniChronyd had an address() method like MiniHms, and the transformation would happen there? Seems a better separation of concerns: options() clearly indicates the input to chronyd, while address() (and perhaps in the future other methods) reflect the running state of chronyd.
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 03 Oct 2019 00:01:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini chronyd] introduce MiniChronyd::address()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14355 )
Change subject: [mini_chronyd] introduce MiniChronyd::address()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14355/2/src/kudu/clock/test/mini_chronyd.cc
File src/kudu/clock/test/mini_chronyd.cc:
http://gerrit.cloudera.org:8080/#/c/14355/2/src/kudu/clock/test/mini_chronyd.cc@176
PS2, Line 176: HostPort addr(options_.bindaddress == kWildcardIpAddr ? kLoopbackIpAddr
: : options_.bindaddress,
: options_.port);
: return addr;
> Nit: could combine.
Done
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 03 Oct 2019 15:50:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini chronyd] introduce MiniChronyd::address()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14355 )
Change subject: [mini_chronyd] introduce MiniChronyd::address()
......................................................................
[mini_chronyd] introduce MiniChronyd::address()
Introduced MiniChronyd::address() method to find IP address and port
at which the underlying NTP server is listening for incoming requests.
In case if the NTP server is bound to the wildcard address, use the
loopback address for NTP communication with the server.
Also, updated corresponding call sites to use the new method instead of
accessing MiniChronyd's options.
Prior to this patch, the ToolTest.TestRemoteReplicaCopy test scenario
would fail if running with built-in NTP client enabled.
Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Reviewed-on: http://gerrit.cloudera.org:8080/14355
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/clock/ntp-test.cc
M src/kudu/clock/test/mini_chronyd-test.cc
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/clock/test/mini_chronyd.h
M src/kudu/mini-cluster/external_mini_cluster.cc
5 files changed, 27 insertions(+), 19 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [mini chronyd] introduce MiniChronyd::address()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14355
to look at the new patch set (#3).
Change subject: [mini_chronyd] introduce MiniChronyd::address()
......................................................................
[mini_chronyd] introduce MiniChronyd::address()
Introduced MiniChronyd::address() method to find IP address and port
at which the underlying NTP server is listening for incoming requests.
In case if the NTP server is bound to the wildcard address, use the
loopback address for NTP communication with the server.
Also, updated corresponding call sites to use the new method instead of
accessing MiniChronyd's options.
Prior to this patch, the ToolTest.TestRemoteReplicaCopy test scenario
would fail if running with built-in NTP client enabled.
Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
---
M src/kudu/clock/ntp-test.cc
M src/kudu/clock/test/mini_chronyd-test.cc
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/clock/test/mini_chronyd.h
M src/kudu/mini-cluster/external_mini_cluster.cc
5 files changed, 27 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14355/3
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [mini chronyd] introduce MiniChronyd::address()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14355
to look at the new patch set (#2).
Change subject: [mini_chronyd] introduce MiniChronyd::address()
......................................................................
[mini_chronyd] introduce MiniChronyd::address()
Introduced MiniChronyd::address() method to find IP address and port
at which the underlying NTP server is listening for incoming requests.
In case if the NTP server is bound to the wildcard address, use the
loopback address for NTP communication with the server.
Also, updated corresponding call sites to use the new method instead of
accessing MiniChronyd's options.
Prior to this patch, the ToolTest.TestRemoteReplicaCopy test scenario
would fail if running with built-in NTP client enabled.
Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
---
M src/kudu/clock/ntp-test.cc
M src/kudu/clock/test/mini_chronyd-test.cc
M src/kudu/clock/test/mini_chronyd.cc
M src/kudu/clock/test/mini_chronyd.h
M src/kudu/mini-cluster/external_mini_cluster.cc
5 files changed, 28 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/14355/2
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [mini chronyd] introduce MiniChronyd::address()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14355 )
Change subject: [mini_chronyd] introduce MiniChronyd::address()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14355/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:
http://gerrit.cloudera.org:8080/#/c/14355/1/src/kudu/mini-cluster/external_mini_cluster.cc@172
PS1, Line 172: ntp_endpoints.emplace_back(
> Would it be clearer if MiniChronyd had an address() method like MiniHms, an
That's much better, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 03 Oct 2019 01:30:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] [mini chronyd] introduce MiniChronyd::address()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14355 )
Change subject: [mini_chronyd] introduce MiniChronyd::address()
......................................................................
Patch Set 3: Code-Review+2
Carrying over Adar's +2 from PS2.
--
To view, visit http://gerrit.cloudera.org:8080/14355
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I62a326a47160aefe9a5806162e6df7c4d6060ba5
Gerrit-Change-Number: 14355
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 03 Oct 2019 16:36:37 +0000
Gerrit-HasComments: No