Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11037 )
Change subject: [Java] Retry tests that don’t inherit from BaseKuduTest ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestSecurity.java@95 PS1, Line 95: public RetryRule retryRule = new RetryRule(); > Why can't TestSecurity extend BaseKuduTest? Can its use of MiniKuduCluster Each test adjust the MiniKuduCluster options a bit. It's likely we could work it in to use it. I would prefer to keep it a different patch though. http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java File java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-client/src/test/java/org/apache/kudu/junit/RetryRule.java@30 PS1, Line 30: private static final String RETRY_PROPERTY_NAME = "rerunFailingTestsCount"; : private static final int DEFAULT_RETRY_COUNT = 0; : private static final String retryPropVal = System.getProperty(RETRY_PROPERTY_NAME); : private static final int retryCount = : retryPropVal != null ? Integer.parseInt(retryPropVal) : DEFAULT_RETRY_COUNT; > I think the entire block would be more readable as: definitely. http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala File java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark-tools/src/test/scala/org/apache/kudu/spark/tools/TestImportExportFiles.scala@32 PS1, Line 32: > Nit: there's an extra space here. Done http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala File java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala: http://gerrit.cloudera.org:8080/#/c/11037/1/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/KuduRDDTest.scala@29 PS1, Line 29: fail() > Did you mean to add this? No, missed one of the places I was testing retries. -- To view, visit http://gerrit.cloudera.org:8080/11037 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I596fde91a13f7e644fd1b1814e9a672f96fa0e4b Gerrit-Change-Number: 11037 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 24 Jul 2018 21:03:00 +0000 Gerrit-HasComments: Yes
