Alexey Serbin has posted comments on this change. Change subject: [kudu-jepsen] Kudu Jepsen tests ......................................................................
Patch Set 16: (19 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 Done Line 5: /store > why do you need '/store' instead of just 'store' or 'store/'? It seems it's a typo -- should be store/ instead. 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, Done PS16, Line 147: liear > typo Done PS16, Line 152: activisation > activation Done PS16, Line 156: correspondning > corresponding Done PS16, Line 158: should under > should be under Done 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 Done 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 Done 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) Done 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) Done 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? Good point: I found it's not necessary here, but some tutorial have (do) in their examples: https://www.tutorialspoint.com/clojure/clojure_while_statement.htm I'll remove that (do) -- at least it seems working without (do). 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 Done PS16, Line 35: respond > response Done 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 clie As I understand, the value associated with the :client key in the result map returned by this function is used to instantiate all clients for the. So, having that atom is the way to store a state between those invocations -- the very first client which sets the atom creates the table, all other clients just open already existing table when jepsen calls setup. 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 Yes, you are right. It seems I wrote this code in an iterative way and missed the point of simplifying it in the very end :) The sequence should not be vec-ified, using list here is OK as well. Basically, it should be just (str/join "/" components) Line 351: ;; Copy appropriate binaries to the node. > can we add some kind of check before copying that the binaries we are going Good idea, but I would like that to work also with binaries from other architecture put into the build/latest/bin: e.g., I use my OS X laptop to copy pre-build Debian/Ubuntu binaries into the DB test nodes. I'll think how to achieve that. 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 ok, passing those transparently would make more sense, yes. 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? Unfortunately, the call of the (jepsen.kudu-test/instatiate-all-kudu-tests test-opts) does not work with locals. So, test-opts should be global, and the rest except iter-num. In that case I think it's better to keep them all global. -- 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
