Adar Dembo has posted comments on this change.

Change subject: WIP [jepsen.kudu] run tests from clojure-maven-plugin
......................................................................


Patch Set 7:

(7 comments)

It's a little tricky navigating across these patches. You might consider 
squashing them all together to review/merge, unless David or someone else has a 
lot of context on the first patch or two and would prefer to see just the delta 
since then.

http://gerrit.cloudera.org:8080/#/c/5551/7/java/kudu-jepsen/.gitignore
File java/kudu-jepsen/.gitignore:

Line 1: .lein-failures
Could you add comments explaining why we need each one?


http://gerrit.cloudera.org:8080/#/c/5551/7/java/kudu-jepsen/README.md
File java/kudu-jepsen/README.md:

Line 16: Note that commas (not spaces) are used to separate the names of the 
machines.
Would it be possible to let the shell expand the node list? For example:

  mvn clojure:run -DtserverNodes="t{0..4}" -DmasterNodes="m0"


Line 22: List of required packages For Debian/Ubuntu machines if us:
Could you reformat this sentence? It has some grammar issues and multiple 
capital letters.


PS7, Line 23:   krb5-admin-server krb5-kdc krb5-user
            :   libsasl2-modules libsasl2-modules-gssapi-mit lsb-release ntp 
openssl
Nit: reformat this into an actual markdown list.


PS7, Line 27: on
of


http://gerrit.cloudera.org:8080/#/c/5551/7/java/kudu-jepsen/pom.xml
File java/kudu-jepsen/pom.xml:

PS7, Line 29:       <masterNodes></masterNodes>
            :       <tserverNodes></tserverNodes>
            :       <sshKeyPath></sshKeyPath>
Why is this necessary? Aren't all properties empty by default?


Line 102:                 <args>--master-nodes=${masterNodes} 
--tserver-nodes=${tserverNodes} --ssh-key-path=${sshKeyPath}</args>
Nit: could you separate each argument on its own line so this is more readable?


-- 
To view, visit http://gerrit.cloudera.org:8080/5551
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5adb6968df46954f94c11f29ecc4dd4ea56544b1
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <danburk...@apache.org>
Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to