Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/12042 )
Change subject: java: add support for flaky test reporting ...................................................................... Patch Set 1: (19 comments) http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/build.gradle File java/kudu-test-utils/build.gradle: http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/build.gradle@35 PS1, Line 35: testCompile libs.junit > junit is already a compile time dependency so it's not needed here. I think Done http://gerrit.cloudera.org:8080/#/c/12042/1/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: PS1: > License header. Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@23 PS1, Line 23: public static class Options { > I was just curious the reasoning since it's a different pattern than the ot I don't have a strong opinion on this so I am leaving it as is unless someone speaks up again. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@69 PS1, Line 69: .reportResults(isReportingEnabled()) > Why not just not create a ResultReporter if this is false? Seems like it'd It's more intuitive to report results if and only if there's a ResultReporter, but if we create a reporter only when results are being reported to some remote server then the value of the ResultReporter member in RetryRule will be null when results aren't being reported. I'd rather embrace the idea that a ResultReporter can no-op report results and gain the guarantee that the ResultReporter member won't be null rather than do the most intuitive thing. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@70 PS1, Line 70: .httpEndpoint(System.getenv("TEST_RESULT_SERVER")) > For this and the rest of the required environment variables, we should vali What's your expected behavior if KUDU_REPORT_TEST_RESULTS is set but one of the required env variables isn't set? Throw? http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@106 PS1, Line 106: private static String tryLookupHostname() { > Yes, for consistency. Is there a Java SDK call for that? https://stackoverflow.com/questions/7348711 seems to be saying this is like calling `hostname`. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@120 PS1, Line 120: // TODO(mpercy): add support for uploading the test log to the server. > I'm worried that not doing this now will yield two sets of data points: one I'm fine with doing this before we push the series but I'd like to do it in a separate patch. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@124 PS1, Line 124: // Construct the HTTP endpoint URL. > This (and the majority of params) is unknown at construction time. We needn I don't think this really matters. There isn't any significant amount of work that we'd be caching. The hostname, which does involve work to get, is cached. http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/main/java/org/apache/kudu/test/junit/ResultReporter.java@142 PS1, Line 142: for (Map.Entry<String, String> entry : params.entrySet()) { > Can you use something URIBuilder (http://hc.apache.org/httpcomponents-clien Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java File java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java: PS1: > License header. Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@27 PS1, Line 27: // Unit test for ResultReporter. > Wrong comment style. Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@49 PS1, Line 49: StringBuilder sb = new StringBuilder(); : for (String s : new String[] { testName, buildId, revision, hostname, : buildConfig, Integer.toString(status) }) { : sb.append(s); : sb.append(" "); : } > Maybe use a Guava Joiner instead? Or Objects.toStringHelper? Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@61 PS1, Line 61: private static final Logger LOGGER = LoggerFactory.getLogger(MockFlakyTestServiceHandler.class); > FYI, our naming convention for Loggers is LOG: Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@62 PS1, Line 62: private List<TestRecord> records = new ArrayList<>(); > Could be final. Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@75 PS1, Line 75: String[] argPairs = request.getQueryString().split("&"); > Can merge this into the following line. Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@78 PS1, Line 78: if (keyVal.length == 2) { > Should we throw if it's not 2? It has to be, right? Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@91 PS1, Line 91: private static final String bindAddr = "127.0.0.1"; > nit: Should this be all caps? Done http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@111 PS1, Line 111: public void test() throws IOException { > I think a test that ensures a downed flaky test endpoint doesn't fail tests `tryReportResult` ensures that failing to report a result doesn't fail the test. I think potentially slowing down the tests is necessary because we have to try to connect to the flaky test service, and we might need to wait the timeout each time. We'd have to refactor the thing to aggregate results and bulk report them at the end. I'd prefer to wait to do that until we have problems (perfect is the enemy of good). http://gerrit.cloudera.org:8080/#/c/12042/1/java/kudu-test-utils/src/test/java/org/apache/kudu/test/junit/TestResultReporter.java@118 PS1, Line 118: .buildId("shark") > The song was stuck in my head yesterday... I've got a niece that's into it /| -- 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: 1 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: Tue, 12 Mar 2019 21:00:01 +0000 Gerrit-HasComments: Yes
