Adar Dembo has posted comments on this change. Change subject: Kudu Jepsen Tests - Initial Commit ......................................................................
Patch Set 6: (11 comments) I only skimmed the Jepsen code; mostly I was looking at how this integrates into the rest of the system. http://gerrit.cloudera.org:8080/#/c/5492/6//COMMIT_MSG Commit Message: PS6, Line 17: WIP as this still needs some cleanup/minimal docs but : should be reasonably ready for a first review iteration. Should probably be updated now that this is going to be committed, right? http://gerrit.cloudera.org:8080/#/c/5492/6/java/kudu-jepsen/.gitignore File java/kudu-jepsen/.gitignore: Line 1: /store What is this for? Perhaps add a comment just before it? http://gerrit.cloudera.org:8080/#/c/5492/6/java/kudu-jepsen/README.md File java/kudu-jepsen/README.md: Let's either rewrite this to be useful or remove it altogether. http://gerrit.cloudera.org:8080/#/c/5492/6/java/kudu-jepsen/pom.xml File java/kudu-jepsen/pom.xml: PS6, Line 26: <properties> : <skipTests>true</skipTests> : </properties> Why this? Add an XML comment to explain. Line 31: Remove this. Line 49: <version>0.1.3</version> I think our convention is to list all of our dependent versions in the main pom.xml rather than in each subproject. Line 56: <exclusions> Why the slf4j-api exclusion here and below? Add a comment explaining. Line 89: <extensions>true</extensions> If this is a default value, can you remove it? If not, could you add a comment explaining why we're using it? http://gerrit.cloudera.org:8080/#/c/5492/6/java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj: Line 17: ;; TODO allow to replace the binaries with locally built ones Yes. I assume Maven is used to launch the Jepsen tests? If so, it would be nice if these were specified as maven properties that could be overridden. Line 74: (c/su So the obvious question is: why do we need to use system packages with Jepsen? If we could use regular old binaries: 1) We could run Jepsen on el platforms too, and 2) I don't think we'd need to be root. http://gerrit.cloudera.org:8080/#/c/5492/6/java/pom.xml File java/pom.xml: Line 308: <!-- Build the jepsen test for Kudu. Why hide the jepsen tests behind a disabled-by-default Maven profile? Is it impossible to build without some custom system configuration? -- To view, visit http://gerrit.cloudera.org:8080/5492 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
