Grant Henke 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 no java class or package that is purely numerical? 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? 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. Alternatively, I am not sure you need a constant for the value 0. It would make the `retryCount != MAYBE_RETRY;` line below easier to read to just hard code 0. 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? 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 KUDU_FLAKY_TEST_LIST could be fairly large, can we populate a map when we initialize the rule and consult it in this method? We could read/parse KUDU_RETRY_ALL_FAILED_TESTS and KUDU_FLAKY_TEST_ATTEMPTS at construction time too, though the impact is likely lower. 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 `actualRetryCount = 0` in the case one of the others isn't also true. -- 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 14:11:04 +0000 Gerrit-HasComments: Yes
