Alexey Serbin has posted comments on this change. Change subject: [kudu-jepsen] Kudu Jepsen tests ......................................................................
Patch Set 12: (13 comments) 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 Done 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 'fla Done 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...) Good catch -- yes, indeed. That's a bug. Fixed. Line 46: (def ntp-common-opts ["statistics loopstats peerstats clockstats" > could you 'slurp' these from a resource file instead? Done PS12, Line 64: masters > I don't think NTP uses the term 'masters'. Probably better to say 'servers' Done Line 66: [(str "server " (name (first masters)) > shouldn't this use all of the configured masters, not just the first? eg so Done PS12, Line 69: (defn db : [] : "Apache Kudu." > src/main/clojure/jepsen/kudu.clj:69:7: misplaced-docstrings: Possibly mispl Done PS12, Line 151: (into [] > https://github.com/bbatsov/clojure-style-guide says to use (vec ...) instea Done 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? Good idea! Done. Line 38: ([name type key?] (.build (.key (new ColumnSchema$ColumnSchemaBuilder name, type) key?)))) > how about: Done PS12, Line 51: columns (.getColumns (.getSchema row-result) > again maybe the java programmer in me but I think (-> row-result .getSchema Done PS12, Line 55: case (.ordinal (.getType column)) : ;; Clojure transforms enums in literals : ;; so we have to use ordinals :( > what about using cond instead? Good idea -- I replaced that with condp and it works. PS12, Line 71: ->t > the style guide I'm looking at says to use -> for "conversion functions" bu Done: renamed into 'drain-scanner-into-tuples' -- 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 <[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
