[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Reviewed-by: Adar Dembo 
---
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] build: enable sharding within cmake/ctest

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 04 Mar 2018 19:44:16 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 04 Mar 2018 19:43:30 +
Gerrit-HasComments: No

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 04 Mar 2018 18:53:56 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sun, 04 Mar 2018 16:51:00 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] build: enable sharding within cmake/ctest

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 03 Mar 2018 01:08:30 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
" which uses 'ctest -R " 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:51:57 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:41:00 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:32:50 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 03 Mar 2018 00:06:56 +
Gerrit-HasComments: Yes

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] build: enable sharding within cmake/ctest

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo