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/11/12 05:51:41 UTC

[kudu-CR] WIP KUDU-2994: add NO CHRONY flag 

Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14691


Change subject: WIP KUDU-2994: add NO_CHRONY flag
......................................................................

WIP KUDU-2994: add NO_CHRONY flag

This patch addresses KUDU-2994, adding NO_CHONY cmake/make flag.
If -DNO_CHRONY=1 is specified with cmake options, then the Kudu
testing framework does not rely on chronyc/chronyd, and no corresponding
binaries are deployed under $BUILD_DIR/bin directory.

The build_mini_cluster_binaries.sh script was updated to include
-DNO_CHRONY=1 option correspondingly.

I verified that the result set of tests passes using dist-test:
  http://dist-test.cloudera.org/job?job_id=aserbin.1573536365.5180

WIP: should we remove all the MiniChronyd as well if NO_CHRONY
     is specified?  I tried to do so, and it works but requires
     added several #ifdefs into external_mini_cluster.{cc,h}.

Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
---
M CMakeLists.txt
M build-support/mini-cluster/build_mini_cluster_binaries.sh
M src/kudu/clock/CMakeLists.txt
M src/kudu/clock/ntp-test.cc
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
6 files changed, 40 insertions(+), 13 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/14691/1
-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>

[kudu-CR] KUDU-2994: add NO CHRONY flag

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14691 )

Change subject: KUDU-2994: add NO_CHRONY flag
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14691/2/src/kudu/clock/CMakeLists.txt
File src/kudu/clock/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/14691/2/src/kudu/clock/CMakeLists.txt@41
PS2, Line 41:   # These are copied/installed instead of linking because:
            :   #  * symlinks would not work with dist-test
            :   #  * hardlinks would not work if the target directory is at different
            :   #    filesystem than thirdparty
            :   file(COPY "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/bin/chronyc"
            :     DESTINATION "${EXECUTABLE_OUTPUT_PATH}")
            :   file(COPY "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/sbin/chronyd"
            :     DESTINATION "${EXECUTABLE_OUTPUT_PATH}")
> May as well revert this indentation, or indent all of L50-L64.
Done


http://gerrit.cloudera.org:8080/#/c/14691/2/src/kudu/clock/CMakeLists.txt@71
PS2, Line 71: if (NOT NO_CHRONY)
> Nit: invert to avoid confusing double negative.
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
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: Tue, 12 Nov 2019 08:32:50 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2994: add NO CHRONY flag

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14691 )

Change subject: KUDU-2994: add NO_CHRONY flag
......................................................................

KUDU-2994: add NO_CHRONY flag

This patch addresses KUDU-2994, adding NO_CHRONY cmake/make flag.
If -DNO_CHRONY=1 is specified with cmake options, then the Kudu testing
framework does not rely on chronyc/chronyd, and no corresponding
binaries are deployed under $BUILD_DIR/bin directory.  Also, the
libmini_chronyd and libmini_chronyd_test_util libraries are not built
and references to MiniChronyd are removed from ExternalMiniCluster
in that case.

The build_mini_cluster_binaries.sh script was updated to include
-DNO_CHRONY=1 option correspondingly.

I verified that the result set of tests passes using dist-test:
  http://dist-test.cloudera.org/job?job_id=aserbin.1573546681.122233

Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Reviewed-on: http://gerrit.cloudera.org:8080/14691
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
M build-support/mini-cluster/build_mini_cluster_binaries.sh
M src/kudu/clock/CMakeLists.txt
M src/kudu/clock/ntp-test.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 107 insertions(+), 35 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified

-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
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] WIP KUDU-2994: add NO CHRONY flag

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14691 )

Change subject: WIP KUDU-2994: add NO_CHRONY flag
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/14691/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14691/1//COMMIT_MSG@9
PS1, Line 9: NO_CHONY
NO_CHRONY


http://gerrit.cloudera.org:8080/#/c/14691/1//COMMIT_MSG@20
PS1, Line 20: WIP: should we remove all the MiniChronyd as well if NO_CHRONY
            :      is specified?  I tried to do so, and it works but requires
            :      added several #ifdefs into external_mini_cluster.{cc,h}.
It's less important because MiniChrony is only loosely coupled with chrony itself, but I suppose it'd still be nice to do from a POLA perspective.


http://gerrit.cloudera.org:8080/#/c/14691/1/build-support/mini-cluster/build_mini_cluster_binaries.sh
File build-support/mini-cluster/build_mini_cluster_binaries.sh:

http://gerrit.cloudera.org:8080/#/c/14691/1/build-support/mini-cluster/build_mini_cluster_binaries.sh@110
PS1, Line 110:   -DNO_TESTS=1 -DNO_CHRONY=1 \
Might want to expand on the comment on L104 to rationalize this.


http://gerrit.cloudera.org:8080/#/c/14691/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/14691/1/src/kudu/mini-cluster/external_mini_cluster.cc@120
PS1, Line 120:       num_ntp_servers(0)
Do we want to allow using the built-in NTP client _without_ MiniChronyd? I don't think we'd ever want to build tests that way (need Internet access, slow to acquire sync, etc.) but could be an interesting option for `kudu test mini_cluster`.

It's not in scope of this change though.



-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
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: Tue, 12 Nov 2019 06:11:09 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2994: add NO CHRONY flag

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14691 )

Change subject: KUDU-2994: add NO_CHRONY flag
......................................................................


Patch Set 3: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
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: Tue, 12 Nov 2019 08:49:58 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2994: add NO CHRONY flag

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/14691

to look at the new patch set (#2).

Change subject: KUDU-2994: add NO_CHRONY flag
......................................................................

KUDU-2994: add NO_CHRONY flag

This patch addresses KUDU-2994, adding NO_CHRONY cmake/make flag.
If -DNO_CHRONY=1 is specified with cmake options, then the Kudu testing
framework does not rely on chronyc/chronyd, and no corresponding
binaries are deployed under $BUILD_DIR/bin directory.  Also, the
libmini_chronyd and libmini_chronyd_test_util libraries are not built
and references to MiniChronyd are removed from ExternalMiniCluster
in that case.

The build_mini_cluster_binaries.sh script was updated to include
-DNO_CHRONY=1 option correspondingly.

I verified that the result set of tests passes using dist-test:
  http://dist-test.cloudera.org/job?job_id=aserbin.1573546681.122233

Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
---
M CMakeLists.txt
M build-support/mini-cluster/build_mini_cluster_binaries.sh
M src/kudu/clock/CMakeLists.txt
M src/kudu/clock/ntp-test.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 94 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/14691/2
-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)

[kudu-CR] KUDU-2994: add NO CHRONY flag

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/14691 )

Change subject: KUDU-2994: add NO_CHRONY flag
......................................................................


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/14691/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/14691/1//COMMIT_MSG@9
PS1, Line 9: NO_CHONY
> NO_CHRONY
Done


http://gerrit.cloudera.org:8080/#/c/14691/1//COMMIT_MSG@20
PS1, Line 20: WIP: should we remove all the MiniChronyd as well if NO_CHRONY
            :      is specified?  I tried to do so, and it works but requires
            :      added several #ifdefs into external_mini_cluster.{cc,h}.
> It's less important because MiniChrony is only loosely coupled with chrony 
Thank you for the feedback.

Done.


http://gerrit.cloudera.org:8080/#/c/14691/1/build-support/mini-cluster/build_mini_cluster_binaries.sh
File build-support/mini-cluster/build_mini_cluster_binaries.sh:

http://gerrit.cloudera.org:8080/#/c/14691/1/build-support/mini-cluster/build_mini_cluster_binaries.sh@110
PS1, Line 110:   -DNO_TESTS=1 -DNO_CHRONY=1 \
> Might want to expand on the comment on L104 to rationalize this.
Done


http://gerrit.cloudera.org:8080/#/c/14691/1/src/kudu/mini-cluster/external_mini_cluster.cc
File src/kudu/mini-cluster/external_mini_cluster.cc:

http://gerrit.cloudera.org:8080/#/c/14691/1/src/kudu/mini-cluster/external_mini_cluster.cc@120
PS1, Line 120:       num_ntp_servers(0)
> Do we want to allow using the built-in NTP client _without_ MiniChronyd? I 
Yep, we could allow for an extra option to specify some external NTP servers to run the built-in NTP client of external mini-cluster tests.  It might be a viable option, but it's slower compared with MiniChronyd and a dedicated chronyd NTP server as a part of the mini-cluster.

I think that behavior might be controlled by an environment variable, where the variable would contain the set of external NTP servers to point the built-in NTP client to.



-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
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: Tue, 12 Nov 2019 08:23:48 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2994: add NO CHRONY flag

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14691 )

Change subject: KUDU-2994: add NO_CHRONY flag
......................................................................


Patch Set 2: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/14691/2/src/kudu/clock/CMakeLists.txt
File src/kudu/clock/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/14691/2/src/kudu/clock/CMakeLists.txt@41
PS2, Line 41:   # These are copied/installed instead of linking because:
            :   #  * symlinks would not work with dist-test
            :   #  * hardlinks would not work if the target directory is at different
            :   #    filesystem than thirdparty
            :   file(COPY "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/bin/chronyc"
            :     DESTINATION "${EXECUTABLE_OUTPUT_PATH}")
            :   file(COPY "${CMAKE_SOURCE_DIR}/thirdparty/installed/common/sbin/chronyd"
            :     DESTINATION "${EXECUTABLE_OUTPUT_PATH}")
May as well revert this indentation, or indent all of L50-L64.


http://gerrit.cloudera.org:8080/#/c/14691/2/src/kudu/clock/CMakeLists.txt@71
PS2, Line 71: if (NOT NO_CHRONY)
Nit: invert to avoid confusing double negative.



-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
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: Tue, 12 Nov 2019 08:28:40 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2994: add NO CHRONY flag

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/14691

to look at the new patch set (#3).

Change subject: KUDU-2994: add NO_CHRONY flag
......................................................................

KUDU-2994: add NO_CHRONY flag

This patch addresses KUDU-2994, adding NO_CHRONY cmake/make flag.
If -DNO_CHRONY=1 is specified with cmake options, then the Kudu testing
framework does not rely on chronyc/chronyd, and no corresponding
binaries are deployed under $BUILD_DIR/bin directory.  Also, the
libmini_chronyd and libmini_chronyd_test_util libraries are not built
and references to MiniChronyd are removed from ExternalMiniCluster
in that case.

The build_mini_cluster_binaries.sh script was updated to include
-DNO_CHRONY=1 option correspondingly.

I verified that the result set of tests passes using dist-test:
  http://dist-test.cloudera.org/job?job_id=aserbin.1573546681.122233

Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
---
M CMakeLists.txt
M build-support/mini-cluster/build_mini_cluster_binaries.sh
M src/kudu/clock/CMakeLists.txt
M src/kudu/clock/ntp-test.cc
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/mini-cluster/external_mini_cluster-test.cc
M src/kudu/mini-cluster/external_mini_cluster.cc
M src/kudu/mini-cluster/external_mini_cluster.h
8 files changed, 107 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/91/14691/3
-- 
To view, visit http://gerrit.cloudera.org:8080/14691
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I95495f8a2418546048e2ea28c4f44cb399f08fd3
Gerrit-Change-Number: 14691
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)