Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12917 )
Change subject: build: adapt new Java flaky test infrastructure to existing controls ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/dist_test.py File build-support/dist_test.py: http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/dist_test.py@369 PS1, Line 369: test_name = ".".join(k.split(".")[:-1]) if TEST_SHARD_RE.search(k) else k > If I understand right, this doesn't mangle the java names because there is Correct. And it looks like every class/package identifier needs to have at least one letter[1], so no Java test should ever match this regex. 1. https://stackoverflow.com/a/65490 http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/12917/1/build-support/jenkins/build-and-test.sh@246 PS1, Line 246: EXTRA_GRADLE_TEST_FLAGS="--tests $t $EXTRA_GRADLE_TEST_FLAGS" > Will this accidentally pass the flaky c++ tests to the Gradle --tests flag? Not accidentally, but yes. :) As I wrote in the commit message, the shortcut that simplified this patch was to _not_ separate the list of C++ and Java flakies. We can get away with it because: 1. There are no collisions between C++/Java test names. 2. Both ctest (with "-R $test_regex") and gradle (with multiple "--tests $t" entries) happily ignore tests they can't find. A cleaner way would be to enforce the separation, but that means doing one of: 1. Splitting the unified flaky list here into two lists based on some name-based criteria. 2. Adding another flaky list retrieval endpoint to test_result_server.py and modifying the SQL queries to do the same name-based criteria. 3. Like #2 but modifying the reporting to include a bool for C++ or Java test, and changing the results schema accordingly. I opted for quick-and-dirty. http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java: http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@51 PS1, Line 51: private static final int MAYBE_RETRY = 0; > Might be more clear to call this NO_EXPLICIT_RETRIES or something. Alternat Done http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@57 PS1, Line 57: this(MAYBE_RETRY, /*skipReporting=*/ false); > Should this be DEFAULT_RETRY_COUNT? Done http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@72 PS1, Line 72: String value = System.getenv("KUDU_FLAKY_TEST_LIST"); > I am not sure how costly these System.getenv calls are, but assuming this K System.getenv() should be cheap; it's not even a system call [1]. I don't think a map would save us any time, as we only call retryThisTest() once per RetryRule. Unless you're suggesting a static map created in a static block. I find static blocks somewhat icky, but it would reduce the number of times we read the file, so maybe it is worth doing. 1. https://stackoverflow.com/questions/46623018/does-a-program-make-a-system-call-to-get-the-value-of-an-environment-variable-in http://gerrit.cloudera.org:8080/#/c/12917/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@128 PS1, Line 128: reporter != null > Do we need this condition? I think below it would always result in `actualR Yes, counter-intuitively it's OK to construct a RetryStatement with actualRetryCount == 0, because that's a test that won't be retried but will report its results. Put another way, reporting and retrying should be independently controllable (see the comment just above). I don't think we take advantage of this particular combination, but the C++ tests support it, so I figured I'd at least reach parity with them. -- To view, visit http://gerrit.cloudera.org:8080/12917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia89598d7eeb5ab642ab4ebb7aa583adcce770eae Gerrit-Change-Number: 12917 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Comment-Date: Wed, 03 Apr 2019 17:51:52 +0000 Gerrit-HasComments: Yes
