You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2018/03/02 23:26:42 UTC
[kudu-CR] build: enable sharding within cmake/ctest
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9470
to review the following change.
Change subject: build: enable sharding within cmake/ctest
......................................................................
build: enable sharding within cmake/ctest
This changes test sharding to be set up via cmake rather than specialized code
in the dist-test wrapper:
* There is a new argument for the ADD_KUDU_TEST() CMake function. When
set, we generate separate CTest executions for each shard of the
test. Thus, 'ctest -j' can now parallelize the multiple shards of
longer-running tests.
* The specialized sharding logic is now gone from dist_test.py. Instead,
it is now able to parse more of the output from 'ctest -N' and grabs
the sharding-related environment variables directly from there and
passes them down into the test environment.
* The setting of the GTEST_OUTPUT environment variable is moved from
build-and-test.sh into run_test.sh since it's necessary to ensure
that the different shards output to separate XML files.
* Flaky-test tracking is currently still done by the test binary name
and not the specific shard, though we could easily switch that in
the future.
Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
M build-support/run-test.sh
M build-support/run_dist_test.py
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
11 files changed, 162 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/9470/1
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 5: Verified+1
Unrelated flaky
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 04 Mar 2018 19:43:30 +0000
Gerrit-HasComments: No
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@442
PS2, Line 442:
> I see. I've often used loop to loop an entire suite. For example, I'd loop
hmm, how about we take a page from ctest and have "dist_test.py loop -n 100 -R <regex>" which uses 'ctest -R <regex>" to find the matching tests? Then I think we'd get the behavior you want but still allow "loop build/latest/bin/foo-test"?
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:51:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@165
PS4, Line 165: cwd=rel_to_abs("build/latest")
> it's actually a bug fix that I noticed when I was playing with this. If you
Nah, no need to break it out.
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 04 Mar 2018 19:44:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (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/9470
to look at the new patch set (#2).
Change subject: build: enable sharding within cmake/ctest
......................................................................
build: enable sharding within cmake/ctest
This changes test sharding to be set up via cmake rather than specialized code
in the dist-test wrapper:
* There is a new argument for the ADD_KUDU_TEST() CMake function. When
set, we generate separate CTest executions for each shard of the
test. Thus, 'ctest -j' can now parallelize the multiple shards of
longer-running tests.
* The specialized sharding logic is now gone from dist_test.py. Instead,
it is now able to parse more of the output from 'ctest -N' and grabs
the sharding-related environment variables directly from there and
passes them down into the test environment.
* The setting of the GTEST_OUTPUT environment variable is moved from
build-and-test.sh into run_test.sh since it's necessary to ensure
that the different shards output to separate XML files.
* Flaky-test tracking is currently still done by the test binary name
and not the specific shard, though we could easily switch that in
the future.
Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
M build-support/run-test.sh
M build-support/run_dist_test.py
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
11 files changed, 163 insertions(+), 125 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/9470/2
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (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/9470
to look at the new patch set (#3).
Change subject: build: enable sharding within cmake/ctest
......................................................................
build: enable sharding within cmake/ctest
This changes test sharding to be set up via cmake rather than specialized code
in the dist-test wrapper:
* There is a new argument for the ADD_KUDU_TEST() CMake function. When
set, we generate separate CTest executions for each shard of the
test. Thus, 'ctest -j' can now parallelize the multiple shards of
longer-running tests.
* The specialized sharding logic is now gone from dist_test.py. Instead,
it is now able to parse more of the output from 'ctest -N' and grabs
the sharding-related environment variables directly from there and
passes them down into the test environment.
* The setting of the GTEST_OUTPUT environment variable is moved from
build-and-test.sh into run_test.sh since it's necessary to ensure
that the different shards output to separate XML files.
* Flaky-test tracking is currently still done by the test binary name
and not the specific shard, though we could easily switch that in
the future.
Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
M build-support/run-test.sh
M build-support/run_dist_test.py
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
11 files changed, 171 insertions(+), 126 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/9470/3
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has removed a vote on this change.
Change subject: build: enable sharding within cmake/ctest
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@442
PS2, Line 442:
> yea, though with looping a test you specify the whole command line yourself
I see. I've often used loop to loop an entire suite. For example, I'd loop raft_consensus-itest 1000 times and wait for 8000 test instances to finish running. Without sharding, I'd get 1000 mega-instances that may individually time out, right?
What if we parsed argv for the test name(s), then looked for them in the output of ctest -N? I guess it'd get pretty annoying if we allowed both sharded and non-sharded test names on the command line.
http://gerrit.cloudera.org:8080/#/c/9470/3/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/3/build-support/dist_test.py@449
PS3, Line 449: argv=(
Is naming the argument actually necessary? Won't the first argument get marshalled into 'argv', the second (which doesn't exist here) into 'env', etc?
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:41:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9470/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9470/2//COMMIT_MSG@26
PS2, Line 26: * Flaky-test tracking is currently still done by the test binary name
: and not the specific shard, though we could easily switch that in
: the future.
> This gets weird though because the number of shards may change over time, w
yea, that's why I didn't change for now.. on the other hand it can be useful to know which sub-shard is actually flaky. I suppose longest term it woudl be nice to even track each test-case separately but that woudl require more mechanics
http://gerrit.cloudera.org:8080/#/c/9470/2/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/9470/2/CMakeLists.txt@678
PS2, Line 678: If a test suite is long enough
: # to require a bumped timeout, consider enabling sharding of the
: # test by adding it to the NUM_SHARDS_BY_TEST dictionary in dist_test.py.
> Update.
Done
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@56
PS2, Line 56: TEST_COMMAND_RE = re.compile('Test command: (.+)$')
: TEST_ENV_RE = re.compile('^\d+: (\S+)=(.+)')
: LDD_RE = re.compile(r'^\s+.+? => (\S+) \(0x.+\)')
> Would be nice to include an example string for each of these.
Done
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@322
PS2, Line 322: name=execution.test_name,
> Note that prior to your change the name of the test also included the total
it seems like this 'name' actually never ends up displayed anywhere best I can tell. It probably corresponds to some ancient version of isolate. I'll just remove it.
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@442
PS2, Line 442: execution = dict(argv=(["run-test.sh", options.cmd] + options.args), env={})
> This isn't right, I think it should be:
yea, though with looping a test you specify the whole command line yourself and typically filter to a single test. In other words, I almost always used the loop mode along with --disable-sharding. I couldn't figure out the best way to modify this for the new scheme. But you're right I also broke the existing functionality.
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:32:50 +0000
Gerrit-HasComments: Yes
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (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/9470
to look at the new patch set (#5).
Change subject: build: enable sharding within cmake/ctest
......................................................................
build: enable sharding within cmake/ctest
This changes test sharding to be set up via cmake rather than specialized code
in the dist-test wrapper:
* There is a new argument for the ADD_KUDU_TEST() CMake function. When
set, we generate separate CTest executions for each shard of the
test. Thus, 'ctest -j' can now parallelize the multiple shards of
longer-running tests.
* The specialized sharding logic is now gone from dist_test.py. Instead,
it is now able to parse more of the output from 'ctest -N' and grabs
the sharding-related environment variables directly from there and
passes them down into the test environment.
A few other changes sprung out of this:
- 'dist-test.py loop' no longer understands shards and thus always
submits a non-sharded test. Its functionality is now available
with sharding in the 'run' subcommand, which is an improved
version of the old 'run-all'.
- the 'run' command can now pass through the '-R' regex filter
parameter to ctest. For example, 'dist_test.py run -R consensus'
will run all tests with consensus in the name
- the 'run' command can also now loop tests with the '-n' flag.
Thus we preserve the ability to loop a sharded test suite.
- the 'run' command can also now tack on extra arguments to the end
of all tests that it submits, which is handy for looping a sharded
test suite with --stress-cpu-threads or other common test flags.
* The setting of the GTEST_OUTPUT environment variable is moved from
build-and-test.sh into run_test.sh since it's necessary to ensure
that the different shards output to separate XML files.
* Flaky-test tracking is currently still done by the test binary name
and not the specific shard, though we could easily switch that in
the future.
Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
M build-support/run-test.sh
M build-support/run_dist_test.py
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
11 files changed, 226 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/9470/5
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 5
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@50
PS4, Line 50: agood
a good
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@165
PS4, Line 165: cwd=rel_to_abs("build/latest")
This is new; why is it needed?
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@470
PS4, Line 470: with the --tests-regex
Nit: "with --tests-regex above" or maybe "with the --tests-regex option above".
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@492
PS4, Line 492: def add_loop_test_subparser(subparsers):
Should this mention that 'loop' is deprecated?
Alternatively, if you're willing to eat the disruption of changing 'run-all' to 'run', perhaps you'd be willing to reduce 'loop' into a warning that instructs people to use 'run' instead of 'loop'?
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 04 Mar 2018 16:51:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 4:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@50
PS4, Line 50: agood
> a good
Done
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@165
PS4, Line 165: cwd=rel_to_abs("build/latest")
> This is new; why is it needed?
it's actually a bug fix that I noticed when I was playing with this. If you don't run dist_test.py from within your build directory, it'll not be able to find the ctests and give you a weird error about having no isolate files to archive. If you want I can break it back out to a separate commit since it actually existed before. It just wasn't as noticeable before because people almost never used the 'run-all' command.
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@470
PS4, Line 470: with the --tests-regex
> Nit: "with --tests-regex above" or maybe "with the --tests-regex option abo
Done
http://gerrit.cloudera.org:8080/#/c/9470/4/build-support/dist_test.py@492
PS4, Line 492: def add_loop_test_subparser(subparsers):
> Should this mention that 'loop' is deprecated?
I actually don't think deprecating loop entirely is a good idea, because usually when I'm running a test locally I don't think about it in terms of shards, and I have no idea which shard a particular test case belongs in. So it's much easier to continue to run 'loop -n 100 build/latest/bin/my-test --gtest_filter=*Foo*' than have to go try to figure out which shard that test belongs to.
I'll clarify the 'NOTE' I put in the loop_test epilog
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sun, 04 Mar 2018 18:53:56 +0000
Gerrit-HasComments: Yes
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
build: enable sharding within cmake/ctest
This changes test sharding to be set up via cmake rather than specialized code
in the dist-test wrapper:
* There is a new argument for the ADD_KUDU_TEST() CMake function. When
set, we generate separate CTest executions for each shard of the
test. Thus, 'ctest -j' can now parallelize the multiple shards of
longer-running tests.
* The specialized sharding logic is now gone from dist_test.py. Instead,
it is now able to parse more of the output from 'ctest -N' and grabs
the sharding-related environment variables directly from there and
passes them down into the test environment.
A few other changes sprung out of this:
- 'dist-test.py loop' no longer understands shards and thus always
submits a non-sharded test. Its functionality is now available
with sharding in the 'run' subcommand, which is an improved
version of the old 'run-all'.
- the 'run' command can now pass through the '-R' regex filter
parameter to ctest. For example, 'dist_test.py run -R consensus'
will run all tests with consensus in the name
- the 'run' command can also now loop tests with the '-n' flag.
Thus we preserve the ability to loop a sharded test suite.
- the 'run' command can also now tack on extra arguments to the end
of all tests that it submits, which is handy for looping a sharded
test suite with --stress-cpu-threads or other common test flags.
* The setting of the GTEST_OUTPUT environment variable is moved from
build-and-test.sh into run_test.sh since it's necessary to ensure
that the different shards output to separate XML files.
* Flaky-test tracking is currently still done by the test binary name
and not the specific shard, though we could easily switch that in
the future.
Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Reviewed-on: http://gerrit.cloudera.org:8080/9470
Tested-by: Todd Lipcon <to...@apache.org>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
M build-support/run-test.sh
M build-support/run_dist_test.py
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
11 files changed, 226 insertions(+), 139 deletions(-)
Approvals:
Todd Lipcon: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 6
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Todd Lipcon (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/9470
to look at the new patch set (#4).
Change subject: build: enable sharding within cmake/ctest
......................................................................
build: enable sharding within cmake/ctest
This changes test sharding to be set up via cmake rather than specialized code
in the dist-test wrapper:
* There is a new argument for the ADD_KUDU_TEST() CMake function. When
set, we generate separate CTest executions for each shard of the
test. Thus, 'ctest -j' can now parallelize the multiple shards of
longer-running tests.
* The specialized sharding logic is now gone from dist_test.py. Instead,
it is now able to parse more of the output from 'ctest -N' and grabs
the sharding-related environment variables directly from there and
passes them down into the test environment.
A few other changes sprung out of this:
- 'dist-test.py loop' no longer understands shards and thus always
submits a non-sharded test. Its functionality is now available
with sharding in the 'run' subcommand, which is an improved
version of the old 'run-all'.
- the 'run' command can now pass through the '-R' regex filter
parameter to ctest. For example, 'dist_test.py run -R consensus'
will run all tests with consensus in the name
- the 'run' command can also now loop tests with the '-n' flag.
Thus we preserve the ability to loop a sharded test suite.
- the 'run' command can also now tack on extra arguments to the end
of all tests that it submits, which is handy for looping a sharded
test suite with --stress-cpu-threads or other common test flags.
* The setting of the GTEST_OUTPUT environment variable is moved from
build-and-test.sh into run_test.sh since it's necessary to ensure
that the different shards output to separate XML files.
* Flaky-test tracking is currently still done by the test binary name
and not the specific shard, though we could easily switch that in
the future.
Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
---
M CMakeLists.txt
M build-support/dist_test.py
M build-support/jenkins/build-and-test.sh
M build-support/run-test.sh
M build-support/run_dist_test.py
M src/kudu/cfile/CMakeLists.txt
M src/kudu/client/CMakeLists.txt
M src/kudu/hms/CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
M src/kudu/tablet/CMakeLists.txt
M src/kudu/tools/CMakeLists.txt
11 files changed, 223 insertions(+), 139 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/70/9470/4
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@442
PS2, Line 442:
> hmm, how about we take a page from ctest and have "dist_test.py loop -n 100
We talked about this offline because I wanted clarification. What you're suggesting is two-fold:
1. Augment 'run-all' with an argument to match test names by regex, and another argument to loop. This is effectively the new (and only) way to run, merging the functionality of both existing commands. The regex can be used to select a particular shard, a subset of shards, a subset of shards, or all shards. It can also be used to select a particular test or subset of tests (orthogonally).
2. Leave 'loop' as a vestigial, deprecated command that only operates on test binaries and isn't aware of sharding. If you need to shard a test to prevent it from timing out, you have to use 'run-all'.
+1
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 03 Mar 2018 01:08:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] build: enable sharding within cmake/ctest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9470 )
Change subject: build: enable sharding within cmake/ctest
......................................................................
Patch Set 2:
(5 comments)
Makes sense, I'm much happier to see the sharding definitions colocated with test definitions.
http://gerrit.cloudera.org:8080/#/c/9470/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/9470/2//COMMIT_MSG@26
PS2, Line 26: * Flaky-test tracking is currently still done by the test binary name
: and not the specific shard, though we could easily switch that in
: the future.
This gets weird though because the number of shards may change over time, which would muck up the sharded test's history.
http://gerrit.cloudera.org:8080/#/c/9470/2/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/9470/2/CMakeLists.txt@678
PS2, Line 678: If a test suite is long enough
: # to require a bumped timeout, consider enabling sharding of the
: # test by adding it to the NUM_SHARDS_BY_TEST dictionary in dist_test.py.
Update.
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py
File build-support/dist_test.py:
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@56
PS2, Line 56: TEST_COMMAND_RE = re.compile('Test command: (.+)$')
: TEST_ENV_RE = re.compile('^\d+: (\S+)=(.+)')
: LDD_RE = re.compile(r'^\s+.+? => (\S+) \(0x.+\)')
Would be nice to include an example string for each of these.
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@322
PS2, Line 322: name=execution.test_name,
Note that prior to your change the name of the test also included the total number of shards. Now it doesn't anymore. Does that matter, or was it purely cosmetic?
http://gerrit.cloudera.org:8080/#/c/9470/2/build-support/dist_test.py@442
PS2, Line 442: execution = dict(argv=(["run-test.sh", options.cmd] + options.args), env={})
This isn't right, I think it should be:
execution = TestExecution([...] + options.args)
Something like that? Actually, this should still call get_test_executions() in some capacity, right? Otherwise looping a sharded test will ignore the sharding.
--
To view, visit http://gerrit.cloudera.org:8080/9470
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I20ddbdd73a64fda3fe32fca98ee541aa4cead4b3
Gerrit-Change-Number: 9470
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:06:56 +0000
Gerrit-HasComments: Yes