You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2019/11/10 10:10:16 UTC
[kudu-CR] WIP: don't use mini chrony in kudu-binary
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/14685
to review the following change.
Change subject: WIP: don't use mini_chrony in kudu-binary
......................................................................
WIP: don't use mini_chrony in kudu-binary
We can't distribute our patched chrony binaries because they're licensed
under the GPLv3. As such, a minicluster started via kudu-binary fails with
an error like this:
java.io.IOException: failed to start NTP server 0: Unable to canonicalize /tmp/kudu-binary-jar14954881897801164502/kudu-binary-1.11.0-linux-x86_64/bin/chronyd: No such file or directory
at org.apache.kudu.test.cluster.MiniKuduCluster.sendRequestToCluster(MiniKuduCluster.java:169)
at org.apache.kudu.test.cluster.MiniKuduCluster.start(MiniKuduCluster.java:234)
To fix this, let's further condition mini_chrony usage in the minicluster
framework on whether the chrony binaries are available or not.
WIP because I have no idea if this makes sense or is the optimal solution; I
just wanted to get something resembling a fix out ASAP.
Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
---
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
3 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/14685/1
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] WIP: KUDU-2994: don't use mini chrony in kudu-binary
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14685 )
Change subject: WIP: KUDU-2994: don't use mini_chrony in kudu-binary
......................................................................
Patch Set 2:
Alexey, if this patch makes sense to you, could you adopt it and see that it gets merged?
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sun, 10 Nov 2019 10:11:22 +0000
Gerrit-HasComments: No
[kudu-CR] WIP: KUDU-2994: don't use mini chrony in kudu-binary
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has abandoned this change. ( http://gerrit.cloudera.org:8080/14685 )
Change subject: WIP: KUDU-2994: don't use mini_chrony in kudu-binary
......................................................................
Abandoned
Alexey's approach in https://gerrit.cloudera.org/c/14691 is better.
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP: KUDU-2994: don't use mini chrony in kudu-binary
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14685 )
Change subject: WIP: KUDU-2994: don't use mini_chrony in kudu-binary
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG@17
PS2, Line 17: To fix this, let's further condition mini_chrony usage in the minicluster
: framework on whether the chrony binaries are available or not.
> Yeah that would be ideal, but how would we do that? New CMake option enable
Yep, I'm planning to post a patch today.
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Nov 2019 18:09:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-2994: don't use mini chrony in kudu-binary
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14685 )
Change subject: WIP: KUDU-2994: don't use mini_chrony in kudu-binary
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG@17
PS2, Line 17: To fix this, let's further condition mini_chrony usage in the minicluster
: framework on whether the chrony binaries are available or not.
> Thank you for for putting together this patch.
Yeah that would be ideal, but how would we do that? New CMake option enabled only in the kudu-binary build? I guess that would work.
Want to give it a shot?
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Nov 2019 16:54:14 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-2994: don't use mini chrony in kudu-binary
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14685
to look at the new patch set (#2).
Change subject: WIP: KUDU-2994: don't use mini_chrony in kudu-binary
......................................................................
WIP: KUDU-2994: don't use mini_chrony in kudu-binary
We can't distribute our patched chrony binaries because they're licensed
under the GPLv3. As such, a minicluster started via kudu-binary fails with
an error like this:
java.io.IOException: failed to start NTP server 0: Unable to canonicalize /tmp/kudu-binary-jar14954881897801164502/kudu-binary-1.11.0-linux-x86_64/bin/chronyd: No such file or directory
at org.apache.kudu.test.cluster.MiniKuduCluster.sendRequestToCluster(MiniKuduCluster.java:169)
at org.apache.kudu.test.cluster.MiniKuduCluster.start(MiniKuduCluster.java:234)
To fix this, let's further condition mini_chrony usage in the minicluster
framework on whether the chrony binaries are available or not.
WIP because I have no idea if this makes sense or is the optimal solution; I
just wanted to get something resembling a fix out ASAP.
Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
---
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
3 files changed, 10 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/85/14685/2
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] WIP: KUDU-2994: don't use mini chrony in kudu-binary
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14685 )
Change subject: WIP: KUDU-2994: don't use mini_chrony in kudu-binary
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG@17
PS2, Line 17: To fix this, let's further condition mini_chrony usage in the minicluster
: framework on whether the chrony binaries are available or not.
Thank you for for putting together this patch.
What do you think if we simply don't enable built-in NTP client for the binaries in kudu-binary artifact since we cannot distribute chrony?
I'm not sure that switching the condition for using system/built-in NTP should automatically change depending on whether chronyd is present. I.e., it could happen that chrony binary will be gone and would not notice some time.
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Nov 2019 04:08:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP: KUDU-2994: don't use mini chrony in kudu-binary
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14685 )
Change subject: WIP: KUDU-2994: don't use mini_chrony in kudu-binary
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/14685/2//COMMIT_MSG@10
PS2, Line 10: GPLv3
BTW, chrony is licensed under GPLv2, not GPLv3: https://github.com/mlichvar/chrony/blob/master/COPYING
I just verified we have that info in our third-party: https://github.com/apache/kudu/blob/master/thirdparty/LICENSE.txt#L689
--
To view, visit http://gerrit.cloudera.org:8080/14685
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2d503634acf837eb3066851dfe9b942e61a77b34
Gerrit-Change-Number: 14685
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Nov 2019 04:13:29 +0000
Gerrit-HasComments: Yes