Todd Lipcon has posted comments on this change. Change subject: [kudu-jepsen] Kudu Jepsen tests ......................................................................
Patch Set 16: (20 comments) http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/.gitignore File java/kudu-jepsen/.gitignore: Line 1: # Ignore files containing information on prior Leiningen runs/sessions. add apache header Line 5: /store why do you need '/store' instead of just 'store' or 'store/'? http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/README.adoc File java/kudu-jepsen/README.adoc: PS16, Line 57: The Kudu master and tserver nodes should be created prior to running the test: : the tests does not create those itself can you clarify whether you just mean that the machines should be created, or whether there should actually be a tserver/master installed? PS16, Line 147: liear typo PS16, Line 152: activisation activation PS16, Line 156: correspondning corresponding PS16, Line 158: should under should be under PS16, Line 162: The Jepsen code is distributed under the Eclipse Public License either : version 1.0 or (at your option) any later version. If we're not shipping Jepsen itself, I dont think it's necessary to specify the license of the Maven dependency Line 165: The kudu-jepsen is licensed under the Apache License, Version 2.0 this should be implicitly covered by the top-level LICENSE file we already have in the repo http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/resources/kudu.flags File java/kudu-jepsen/resources/kudu.flags: Line 1: # This overrides all flags in a flag file nit: add license (or add to rat excludes if not possible) http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/resources/ntp.conf.common File java/kudu-jepsen/resources/ntp.conf.common: Line 1: tinker panic 0 same (license) http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj: PS16, Line 92: do (let [rr-iter (.nextRows scanner)] : (while (.hasNext rr-iter) : (do are these two 'do's necessary given the single-expression loop bodies? http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj: PS16, Line 34: respond response PS16, Line 35: respond response http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj: PS16, Line 71: client (atom false) nil nil) hrm, not entirely following this here. Does this actually run once per client? If so, do you want them to be sharing an atom? it seems like it ends up creating a separate atom per client, in which case the locking and CAS on it wouldn't do anything? PS16, Line 73: ;; (count (:tservers test)) : ;; :num_replicas 1 remove this commented-out code http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/src/main/clojure/jepsen/kudu/util.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu/util.clj: PS16, Line 31: reduce (fn [col e] (conj col e)) [] components) can you explain this to me? not following. isn't this equivalent to (reduce conj [] components)? And isn't that equivalent to just (vec components)? (and not sure why it has to be 'vec'ified anyway)? Line 351: ;; Copy appropriate binaries to the node. can we add some kind of check before copying that the binaries we are going to copy are not a shared-lib build? eg run 'ldd' and check that it doesn't have dependencies in the build dir? http://gerrit.cloudera.org:8080/#/c/5492/16/java/kudu-jepsen/src/utils/kudu_test_runner.clj File java/kudu-jepsen/src/utils/kudu_test_runner.clj: PS16, Line 82: {:masters (:master-nodes options) : :tservers (:tserver-nodes options) : :ssh-key-path (:ssh-key-path options) : :iter-num (:iter-num options)})) seems odd to be mapping the 'options' map to new keys here, and then again map to new keys on line 88-91 below PS16, Line 88: def cmd-line-opts (get-cmd-line-opts)) : (def private-key-path (:ssh-key-path cmd-line-opts)) : (def iter-num (:iter-num cmd-line-opts)) : (def test-opts (dissoc cmd-line-opts :ssh-key-path :iter-num)) would 'let' be better here? -- 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: 16 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
