Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/12042/4/java/gradle/dependencies.gradle File java/gradle/dependencies.gradle: http://gerrit.cloudera.org:8080/#/c/12042/4/java/gradle/dependencies.gradle@96 PS4, Line 96: httpCore : "org.apache.httpcomponents:httpcore:$versions.httpCore", > I don't think you need this, because httpClient will always pull in the app I wasn't sure whether we wanted to explicitly list all direct dependencies, or whether we're comfortable consuming them transitively. I'll remove this. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java: http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java@99 PS4, Line 99: return appended.toString(); > This won't return the text in the case of a file? No, which is yet another reason why separating the file logic into a separate appender is compelling. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/CapturingLogAppender.java@121 PS4, Line 121: public void setWriteToFile(File fileName, boolean useGzip) throws IOException { > I wonder if this should be a new class instead of trying to fit file logic I went back and forth on this. IIRC I settled on the combined appender because I had a case where I always wanted to set it up, but I only sometimes wanted to log to a file. But, I ended up cleaning up ResultReporter further, so that case no longer exists. I'll split this up. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java File java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java: http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@44 PS4, Line 44: public class ResultReporter { > Add audience annotations. Done http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@90 PS4, Line 90: private static final Logger LOG = LoggerFactory.getLogger(ResultReporter.class); > nit: I usually put logging at the very top. Done http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@142 PS4, Line 142: private static String getEnvStringWithDefault(String name, String defaultValue) { > Are these withDefault methods used? Could/Should the be used on KUDU_REPORT I think Will may have added them but forgot to use them. Yeah, I can wire them up to KUDU_REPORT_TEST_RESULTS_VAR and TEST_RESULT_SERVER_VAR. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@156 PS4, Line 156: static String getLocalHostname() { > I am curious why you didn't use `InetAddress.getLocalHost().getHostName()` See previous discussion in this change between Will and myself. tl;dr: it does a reverse DNS lookup, which is very different than calling `hostname`. I'll add a comment. http://gerrit.cloudera.org:8080/#/c/12042/4/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/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@a44 PS4, Line 44: > I think this comment is still true. Oh yeah, at some point I had expressed this via @InterfaceAudience but then lost that. Will fix. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@35 PS4, Line 35: * We use this with Gradle because it doesn't support > I suppose we could update this to remove "with Gradle" since the Maven buil Done http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@56 PS4, Line 56: public RetryRule(int retryCount, boolean enableReporting) { > Can this be called skipReporting? Since the reporting itself is enabled/dis Done http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@88 PS4, Line 88: File tempLogFile = File.createTempFile("test_attempt", ".txt.gz"); > Should the filename include the attempt number? It doesn't really matter since it'll have a different filename when sent to the server, but sure I can add it. http://gerrit.cloudera.org:8080/#/c/12042/4/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/RetryRule.java@91 PS4, Line 91: capturer.setLayout(new PatternLayout("%d{HH:mm:ss.SSS} [%p - %t] (%F:%L) %m%n")); > use the PATTERN_LAYOUT variable above? Oops, done. -- To view, visit http://gerrit.cloudera.org:8080/12042 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34f88363dbf52c2f7bba50fbfb5de059f88e7f74 Gerrit-Change-Number: 12042 Gerrit-PatchSet: 4 Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Thu, 28 Mar 2019 04:24:27 +0000 Gerrit-HasComments: Yes
