David Ribeiro Alves has posted comments on this change. Change subject: Kudu Jepsen Tests - Initial Commit ......................................................................
Patch Set 6: (11 comments) 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? Done 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? Done 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. removed this, for now 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. Done Line 31: > Remove this. Done Line 49: <version>0.1.3</version> > I think our convention is to list all of our dependent versions in the main that is not true. see kudu-spark, kudu-flume-sink etc. We do, however declare the versions on a <properties/> section. Done that. Line 56: <exclusions> > Why the slf4j-api exclusion here and below? Add a comment explaining. Done Line 89: <extensions>true</extensions> > If this is a default value, can you remove it? If not, could you add a comm its disabled by default, added a comment 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 This is an initial version of the jepsen test runs that only runs against the nightlies. I'd rather add that when we support running against different kudu versions. We should decide then what is the best way to define kudu repo locations. Line 74: (c/su > So the obvious question is: why do we need to use system packages with Jeps For expediency, mostly. However I don't think there is much to gain from running jepsen tests against different platforms. I'd rather have a first version that runs against already-provisioned debian/ubuntu nodes and consider whether it's worth it to add support for different platforms afterwards (which, at least for now, I can't think of a reason why it'd be worthwhile) 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 yeah, I agree with Adar here. these should be built by default, even if the tests are not run. I removed this. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
