You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "Yingchun Lai (Code Review)" <ge...@cloudera.org> on 2023/08/08 07:33:45 UTC

[kudu-CR] [tools] Enable 'kudu test' CLI tool when -DNO TESTS=1

Yingchun Lai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20326


Change subject: [tools] Enable 'kudu test' CLI tool when -DNO_TESTS=1
......................................................................

[tools] Enable 'kudu test' CLI tool when -DNO_TESTS=1

'kudu test' CLI tool is useful to start a mini Kudu cluster
for application developing and testing, for example the
examples/java/java-example will start a mini cluster for
testing.

This patch enable to build 'kudu test' CLI tool even if
-DNO_TESTS=1.

Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
---
M src/kudu/clock/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/postgres/CMakeLists.txt
M src/kudu/ranger-kms/CMakeLists.txt
M src/kudu/ranger/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
M src/kudu/tools/tool_action.h
M src/kudu/tools/tool_action_test.cc
M src/kudu/tools/tool_main.cc
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
14 files changed, 79 insertions(+), 124 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
Gerrit-Change-Number: 20326
Gerrit-PatchSet: 1
Gerrit-Owner: Yingchun Lai <la...@apache.org>

[kudu-CR] [tools] Enable 'kudu test' CLI tool when -DKUDU CLI TOOL NO TESTS=0

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Hello Alexey Serbin, Yifan Zhang, Kudu Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/20326

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

Change subject: [tools] Enable 'kudu test' CLI tool when -DKUDU_CLI_TOOL_NO_TESTS=0
......................................................................

[tools] Enable 'kudu test' CLI tool when -DKUDU_CLI_TOOL_NO_TESTS=0

'kudu test' CLI tool is useful to start a mini Kudu cluster
for application developing and testing, for example the
examples/java/java-example will start a mini cluster for
testing.

This patch enable to build 'kudu test' CLI tool when
-DKUDU_CLI_TOOL_NO_TESTS=0 even if -DNO_TESTS=1.

Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
---
M CMakeLists.txt
M build-support/mini-cluster/build_mini_cluster_binaries.sh
M src/kudu/clock/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/master/CMakeLists.txt
M src/kudu/mini-cluster/CMakeLists.txt
M src/kudu/postgres/CMakeLists.txt
M src/kudu/ranger-kms/CMakeLists.txt
M src/kudu/ranger/CMakeLists.txt
M src/kudu/security/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
M src/kudu/tserver/CMakeLists.txt
M src/kudu/util/CMakeLists.txt
13 files changed, 107 insertions(+), 97 deletions(-)


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
Gerrit-Change-Number: 20326
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>

[kudu-CR] [tools] Enable building 'kudu test' CLI tool by default

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20326 )

Change subject: [tools] Enable building 'kudu test' CLI tool by default
......................................................................


Patch Set 5: Code-Review+2

Thanks a lot for this improvement, Yingchun!


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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
Gerrit-Change-Number: 20326
Gerrit-PatchSet: 5
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Tue, 22 Aug 2023 18:39:40 +0000
Gerrit-HasComments: No

[kudu-CR] [tools] Enable 'kudu test' CLI tool when -DKUDU CLI TOOL NO TESTS=0

Posted by "Yifan Zhang (Code Review)" <ge...@cloudera.org>.

Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20326 )

Change subject: [tools] Enable 'kudu test' CLI tool when -DKUDU_CLI_TOOL_NO_TESTS=0
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20326/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/20326/3/CMakeLists.txt@241
PS3, Line 241: The test-related CLI tool will be omitted if not specified or disabled explicitly.
IIUC, the default value of KUDU_CLI_TOOL_NO_TESTS is true when NO_TESTS is set to true. Do you think it is more intuitive that we change it to 'KUDU_CLI_TOOL_WITH_TESTS', it is disabled by default and we should explicitly enable it if needed.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
Gerrit-Change-Number: 20326
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 21 Aug 2023 05:05:49 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] Enable 'kudu test' CLI tool when -DKUDU CLI TOOL NO TESTS=0

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20326 )

Change subject: [tools] Enable 'kudu test' CLI tool when -DKUDU_CLI_TOOL_NO_TESTS=0
......................................................................


Patch Set 2:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/20326/1//COMMIT_MSG@14
PS1, Line 14: This patch enable to build 'kudu test' CLI tool when
            : -DKUDU_CLI_TO
> So, if people are working on Kudu development, they are to build and run te
Yifan found this issue [1] when she verified the 1.17.0-RC1 java example, the java example will try to use Kudu CLI tool to start a mini-cluster by using 'kudu test', it will report an error because the published tool is built with '-DNO_TESTS=1' flag [2] and didn't support 'kudu test'.
An alternative way to resolve this is to remove the '-DNO_TESTS=1' flag in the script, but too many unnecessary test binaries will be built when build the mini cluster binaries, and the 'KUDU_CLI_TOOL_NO_TESTS' flag is not well used.

1. https://lists.apache.org/thread/5s27j7k51xfg06l8nkxq62ooqq5s0m47
2. https://github.com/apache/kudu/blob/master/build-support/mini-cluster/build_mini_cluster_binaries.sh#L126C20-L126C20



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
Gerrit-Change-Number: 20326
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 10 Aug 2023 02:29:58 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] Enable 'kudu test' CLI tool when -DKUDU CLI TOOL NO TESTS=0

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20326 )

Change subject: [tools] Enable 'kudu test' CLI tool when -DKUDU_CLI_TOOL_NO_TESTS=0
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20326/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/20326/3/CMakeLists.txt@241
PS3, Line 241: The test-related CLI tool will be omitted if not specified or disabled explicitly.
> Hi Yifan, thanks for your replay!
I agree with you: too many NO_xxx makes it harder to comprehend, but keeping keeping legacy behavior is a nice thing to have.

What if by default the kudu CLI binary will include the 'test' sub-command even with NO_TESTS=1?  Then there isn't a need to update the commands in the Docker build, and people who want to avoid building anything related to tests and mini-cluster libraries will need to add -DCLI_TEST_TOOL_ENABLED=0 or something in addition to -DNO_TESTS=1 (and that's totally fine, because that's quite rare and most likely that would be some scripts and/or Jenkins job configs).  By default, the cmake's variable CLI_TEST_TOOL_ENABLED will be set to 1, and corresponding macro (say, KUDU_CLI_TEST_TOOL_ENABLED) will be defined as well.

Basically, I'm suggesting to get back to having the 'kudu test' by default when building with -DNO_TESTS=1.

What do you think?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
Gerrit-Change-Number: 20326
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 21 Aug 2023 19:12:52 +0000
Gerrit-HasComments: Yes

[kudu-CR] [tools] Enable 'kudu test' CLI tool when -DKUDU CLI TOOL NO TESTS=0

Posted by "Yingchun Lai (Code Review)" <ge...@cloudera.org>.

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20326 )

Change subject: [tools] Enable 'kudu test' CLI tool when -DKUDU_CLI_TOOL_NO_TESTS=0
......................................................................

Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20326/3/CMakeLists.txt
File CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/20326/3/CMakeLists.txt@241
PS3, Line 241: The test-related CLI tool will be omitted if not specified or disabled explicitly.
> IIUC, the default value of KUDU_CLI_TOOL_NO_TESTS is true when NO_TESTS is
Hi Yifan, thanks for your replay!
Indeed, the the two flags are a bit of counter-intuitive because they are named as 'NO_xxx'.
1. Naming.
The NO_TESTS is an legacy, and the naming of KUDU_CLI_TOOL_NO_TESTS is aim to keep consistency to NO_TESTS.
2. Usage cases and habits.
For the most used two cases,
a. Build for developing purpose, the cmake looks like:

cmake ../.. # all binaries including tests binaries, 'kudu test' tool will be built.

b. Build for publish Kudu server binaries purpose:

cmake -DNO_TESTS=1 ../.. # only Kudu server binaries will be built.

All the commands are kept as before, and the newly added intuitive flag 'KUDU_CLI_TOOL_NO_TESTS' will ony be used in rare cases such as when the release manager build and push mini-cluster binaries.

If change it to KUDU_CLI_TOOL_WITH_TESTS, for the purpose of (a), we have to change the cmake command to

cmake -DKUDU_CLI_TOOL_WITH_TESTS=1 ../..

explicitly to ensure the 'kudu test' tool can be built, which break the old habit.

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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I381348b0123f8566d547cdd4449e308ea5f6aa13
Gerrit-Change-Number: 20326
Gerrit-PatchSet: 3
Gerrit-Owner: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Mon, 21 Aug 2023 06:52:31 +0000
Gerrit-HasComments: Yes