Todd Lipcon has posted comments on this change. Change subject: Kudu Jepsen Tests - Initial Commit ......................................................................
Patch Set 12: (18 comments) http://gerrit.cloudera.org:8080/#/c/5492/12/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: PS12, Line 369: /opt/apache-maven-3.3.9/bin/mvn > If this path is used twice already, may be introduce a variable for that an would prefer not to hard-code any paths to maven here. Instead we should probably just change our job config to put maven 3.3 on the path first and say that maven 3.3 is required. http://gerrit.cloudera.org:8080/#/c/5492/12/java/kudu-jepsen/resources/kudu.flags File java/kudu-jepsen/resources/kudu.flags: PS12, Line 2: --fromenv=rpc_bind_addresses : --fromenv=log_dir hrm, why are these grabbed from the env? http://gerrit.cloudera.org:8080/#/c/5492/12/java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj: Line 1: (ns jepsen.kudu nit: need license headers on this and all other files in this commit Line 17: ;; TODO allow to replace the binaries with locally built ones is this fixed in a later patch in the series? definitely seems fairly important to fix so that the tests in the repo actually test the code in the repo (especially given the below URL isn't publicly accessible afaik) Line 28: (def flags ["--fs_wal_dir=/var/lib/kudu/master" eastwood lint says this is a "def-in-def" which is bad (since it makes 'flags' a global) PS12, Line 32: (when (> (count (:masters test)) 1) : (conj flags (str "--master_addresses=" (concatenate-addresses (:masters test))))) confused by this - it seems like a no-op, since the result of the (when...) expression isn't assigned anywhere, and 'flags' isn't mutated by the (conj) Line 46: (def ntp-common-opts ["statistics loopstats peerstats clockstats" could you 'slurp' these from a resource file instead? PS12, Line 64: masters I don't think NTP uses the term 'masters'. Probably better to say 'servers' or 'ntp-servers' or something. Line 66: [(str "server " (name (first masters)) shouldn't this use all of the configured masters, not just the first? eg something like: (concat ntp-common-opts (map #(str "server " (name %) "burst iburst ...") masters)) PS12, Line 69: (defn db : [] : "Apache Kudu." src/main/clojure/jepsen/kudu.clj:69:7: misplaced-docstrings: Possibly misplaced docstring, db src/main/clojure/jepsen/kudu.clj:69:1: unused-ret-vals: Constant value is discarded: "Apache Kudu." PS12, Line 123: (meh (->> (c/exec :service :kudu-master :stop))) : (meh (->> (c/exec :rm :-rf "/var/lib/kudu/master")))) : : (when (.contains (:tservers test) node) : (info node "Stopping Kudu Tablet Server") : (meh (->> (c/exec :service :kudu-tserver :stop))) : (meh (->> (c/exec :rm :-rf "/var/lib/kudu/tserver"))))) src/main/clojure/jepsen/kudu.clj:123:16: suspicious-expression: ->> called with 1 args. (->> x) always returns x. Perhaps there are misplaced parentheses? src/main/clojure/jepsen/kudu.clj:124:16: suspicious-expression: ->> called with 1 args. (->> x) always returns x. Perhaps there are misplaced parentheses? src/main/clojure/jepsen/kudu.clj:128:16: suspicious-expression: ->> called with 1 args. (->> x) always returns x. Perhaps there are misplaced parentheses? src/main/clojure/jepsen/kudu.clj:129:16: suspicious-expression: ->> called with 1 args. (->> x) always returns x. Perhaps there are misplaced parentheses? PS12, Line 151: (into [] https://github.com/bbatsov/clojure-style-guide says to use (vec ...) instead of (into [] ...) http://gerrit.cloudera.org:8080/#/c/5492/12/java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj: Line 37: ([name type] (.build (.key (new ColumnSchema$ColumnSchemaBuilder name, type) false))) can this be defined as (column-schema name type false) to avoid redundancy? Line 38: ([name type key?] (.build (.key (new ColumnSchema$ColumnSchemaBuilder name, type) key?)))) how about: ([name type key?] (-> (new ColumnSchema$ColumnSchemaBuilder name, type) (.key key?) .build)) to avoid the nesting? PS12, Line 51: columns (.getColumns (.getSchema row-result) again maybe the java programmer in me but I think (-> row-result .getSchema .getColumns) is more readable? PS12, Line 55: case (.ordinal (.getType column)) : ;; Clojure transforms enums in literals : ;; so we have to use ordinals :( what about using cond instead? (let [name (.getName column) type (.getType column) value (cond (= type Type/INT32) (.getInt row-result idx) (= type Type/INT64) (.getLong row-result idx) ...)]) PS12, Line 71: ->t the style guide I'm looking at says to use -> for "conversion functions" but I don't think this is such a function. http://gerrit.cloudera.org:8080/#/c/5492/12/java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj File java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj: Line 11: (defn replace-nodes not really understading this function -- 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: 12 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: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
