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 2017/08/24 21:33:42 UTC
[kudu-CR] Bump test timeout for flex partitioning-itest
Hello Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7814
to review the following change.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Bump test timeout for flex_partitioning-itest
In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.
This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.
Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.
Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
2 files changed, 40 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/7814/1
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7814
to look at the new patch set (#2).
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Bump test timeout for flex_partitioning-itest
In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.
This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.
Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.
Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
---
M CMakeLists.txt
M src/kudu/integration-tests/CMakeLists.txt
2 files changed, 40 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/7814/2
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
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] Bump test timeout for flex partitioning-itest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7814/2/CMakeLists.txt
File CMakeLists.txt:
Line 673: set(ARG_TIMEOUT 900)
> Maybe a comment about how this should be kept in-sync with the default valu
Done
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7814/2/CMakeLists.txt
File CMakeLists.txt:
Line 673: set(ARG_TIMEOUT 900)
Maybe a comment about how this should be kept in-sync with the default value in run-test.sh, and a comment there too?
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Patch Set 3: Verified+1
Release build hit KUDU-2089 (java test retry left tmp files, causing the build to fail)
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: No
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Bump test timeout for flex_partitioning-itest
In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.
This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.
Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.
Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Reviewed-on: http://gerrit.cloudera.org:8080/7814
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Todd Lipcon <to...@apache.org>
---
M CMakeLists.txt
M build-support/run-test.sh
M src/kudu/integration-tests/CMakeLists.txt
3 files changed, 46 insertions(+), 4 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Todd Lipcon: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Patch Set 1:
(3 comments)
Test failure is weird; perhaps an oom kill on the Gradle daemon?
http://gerrit.cloudera.org:8080/#/c/7814/1/CMakeLists.txt
File CMakeLists.txt:
Line 673: set(ARG_TIMEOUT 900)
Can we avoid defining this default value both here and in run-test.sh? Given that we want to support running tests at multiple levels (running the binary directly, running it through run-test.sh, running it through ctest, running it through dist-test) this may not be possible.
If you don't use the ctest timeout, you could condition all of the environment changes on ARG_TIMEOUT, which means the test timeout will be some custom value (if set in CMakeLists.txt) or the default value from run-test.sh (if not). Then there won't be a need to define this here.
Line 707: # Set the ctest timeout to be a bit longer than the timeout we pass to
Why bother with the ctest timeout at all, given that the run-test.sh timeout (respected by every test, dist-test or otherwise) is shorter? Are you worried about deadlocks/hangs induced by run-test.sh's post-processing?
Line 714: if("${CUR_TEST_ENV}" STREQUAL "NOTFOUND")
does if(NOT CUR_TEST_ENV) work?
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: No
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/7814/1/CMakeLists.txt
File CMakeLists.txt:
Line 673: set(ARG_TIMEOUT 900)
> Can we avoid defining this default value both here and in run-test.sh? Give
Yea, I considered doing this (not passing any TIMEOUT if there is nothing set). However I thought it was nice to have the "belt and suspenders" timeout structure here -- if for some reason the KuduTest timeout mechanism gets stuck, it's nice to have ctest applying a timeout as well.
This isn't totally academic - we've seen cases where the 'pstack' which comes when a test timeout hits some bug in gdb which makes it hang, and then the entire build fails. So having ctest kill the test 30 seconds later would prevent that.
Line 707: # Set the ctest timeout to be a bit longer than the timeout we pass to
> Why bother with the ctest timeout at all, given that the run-test.sh timeou
yea, see above
Line 714: if("${CUR_TEST_ENV}" STREQUAL "NOTFOUND")
> does if(NOT CUR_TEST_ENV) work?
ah, I think it might, will try that.
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
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-HasComments: Yes
[kudu-CR] Bump test timeout for flex partitioning-itest
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7814
to look at the new patch set (#3).
Change subject: Bump test timeout for flex_partitioning-itest
......................................................................
Bump test timeout for flex_partitioning-itest
In precommit runs, this test is sharded 8 ways so it never has issues with
timeouts. However, in some Jenkins builds that don't use dist-test,
it can time out, especially when TSAN is enabled.
This adds functionality to our ADD_KUDU_TEST() cmake function to specify
a timeout and plumbs that timeout through to our test wrapper shell
script.
Tested by verifying that the appropriate environment variable gets set
in the environment of the running test.
Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
---
M CMakeLists.txt
M build-support/run-test.sh
M src/kudu/integration-tests/CMakeLists.txt
3 files changed, 46 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/14/7814/3
--
To view, visit http://gerrit.cloudera.org:8080/7814
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I07661415d3500dfa5d7984b720b7a6f14597cc19
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
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>