[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 12:

> Hrm, it sounds like the subsequent patch rewrites/improves a lot of
 > what's in this one. Why not squash the patches so that reviewers
 > don't have to spend time looking at stuff that's already been
 > removed/changed?

OK, that sounds good to me -- I hope David does not mind.  Originally we have 
several patches because we were working on different things independently, but 
now we can merge that.

-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 12:

Hrm, it sounds like the subsequent patch rewrites/improves a lot of what's in 
this one. Why not squash the patches so that reviewers don't have to spend time 
looking at stuff that's already been removed/changed?

-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Dan Burkert (Code Review)]

Dan Burkert has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 12:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5492/12/build-support/jenkins/build-and-test.sh
File build-support/jenkins/build-and-test.sh:

Line 90: echo "Outputting java 8 home"
I agree with Adar's comment on rev 9 about removing or consolidating this to 1 
line.


PS12, Line 358: /opt/apache-maven-3.3.9/bin
> Are we ready to migrate to the new maven to build everything Java?
I tend to agree - just because we use JDK 8 to compile kudu-client and the rest 
doesn't mean they lose compatibility with JRE 7.  The -target flag to javac & 
maven can ensure compat.

Even moreso with bumping maven - as far as I know there aren't any hazards to 
packaging with maven 3.3 which would make the jar incompatible with maven 3.2 
or below.


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 12:

(1 comment)

BTW, what would be the best way to update this patch given the fact there are 
subsequent ones?

I would suggest to fix only issues which are not addressed in the subsequent 
patches.  Does it sound reasonable?

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:

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 
The suspect is gone in the subsequent patch (review item 5500).


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 12:

(4 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
> would prefer not to hard-code any paths to maven here. Instead we should pr
As I see it, this file should not be modified for the first iteration -- we 
discussed that with Adar and the proposed solution would be just build the 
kudu-jepsen module only for special profile -- that's to use in the script for 
the Jenkins job (that's already done).


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?
This is gone -- in the subsequent change I removed that:

https://gerrit.cloudera.org/#/c/5500


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 17: ;; TODO allow to replace the binaries with locally built ones
> is this fixed in a later patch in the series? definitely seems fairly impor
That's done in my subsequent patch:

https://gerrit.cloudera.org/#/c/5500


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
It seems like a hack to me too.  As I understand, this is a hack to filter 
tablet server nodes from the array of all nodes (the latter contains master as 
well).  I removed this hack in my subsequent patch (review item 5500).


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Todd Lipcon (Code Review)]

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
 (= 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 12:

(3 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 358: /opt/apache-maven-3.3.9/bin
Are we ready to migrate to the new maven to build everything Java?

If doing so, why not to transition to Java8 for everything as well?

>From the other side, if we are using Java8 and just for kudu-jepsen, why not 
>to leave it as a special profile/configuration?  At least, that works right 
>now, and since we are not about to run kudu-jepsen as a pre-commit 
>verification test, having a separate profile for kudu-jepsen makes sense IMO.


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 and 
use it everywhere?


http://gerrit.cloudera.org:8080/#/c/5492/12/build-support/jenkins/toolchains.xml
File build-support/jenkins/toolchains.xml:

PS12, Line 13:  
an extra-space


-- 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5492

to look at the new patch set (#12).

Change subject: Kudu Jepsen Tests - Initial Commit
..

Kudu Jepsen Tests - Initial Commit

This patch contains the basic jepsen tests code as it
was before KUDU-798 was resolved (i.e. as it was when it
was failing).

The clojure code is integrated into the Kudu maven build
and is compiled along with the other projects. Tests are
currently skipped as they require tests infrastructure.

Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
---
M build-support/jenkins/build-and-test.sh
A build-support/jenkins/toolchains.xml
A java/kudu-jepsen/.gitignore
A java/kudu-jepsen/pom.xml
A java/kudu-jepsen/resources/kudu.flags
A java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/table.clj
A java/kudu-jepsen/src/test/clojure/jepsen/kudu_test.clj
M java/pom.xml
12 files changed, 563 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5492/12
-- 
To view, visit http://gerrit.cloudera.org:8080/5492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5492

to look at the new patch set (#11).

Change subject: Kudu Jepsen Tests - Initial Commit
..

Kudu Jepsen Tests - Initial Commit

This patch contains the basic jepsen tests code as it
was before KUDU-798 was resolved (i.e. as it was when it
was failing).

The clojure code is integrated into the Kudu maven build
and is compiled along with the other projects. Tests are
currently skipped as they require tests infrastructure.

Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
---
M build-support/jenkins/build-and-test.sh
A build-support/jenkins/toolchains.xml
A java/kudu-jepsen/.gitignore
A java/kudu-jepsen/pom.xml
A java/kudu-jepsen/resources/kudu.flags
A java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/table.clj
A java/kudu-jepsen/src/test/clojure/jepsen/kudu_test.clj
M java/pom.xml
12 files changed, 561 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5492/11
-- 
To view, visit http://gerrit.cloudera.org:8080/5492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5492

to look at the new patch set (#10).

Change subject: Kudu Jepsen Tests - Initial Commit
..

Kudu Jepsen Tests - Initial Commit

This patch contains the basic jepsen tests code as it
was before KUDU-798 was resolved (i.e. as it was when it
was failing).

The clojure code is integrated into the Kudu maven build
and is compiled along with the other projects. Tests are
currently skipped as they require tests infrastructure.

Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
---
M build-support/jenkins/build-and-test.sh
A build-support/jenkins/toolchains.xml
A java/kudu-jepsen/.gitignore
A java/kudu-jepsen/pom.xml
A java/kudu-jepsen/resources/kudu.flags
A java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/table.clj
A java/kudu-jepsen/src/test/clojure/jepsen/kudu_test.clj
M java/pom.xml
12 files changed, 560 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5492/10
-- 
To view, visit http://gerrit.cloudera.org:8080/5492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 9:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5492/9/build-support/jenkins/toolchains.xml
File build-support/jenkins/toolchains.xml:

Line 4:   
> OK, but effect does this file have? Would be nice to explain in this commen
this is my attempt at getting the build to use jdk 8 just for the jepsen 
module. it requires pushing this stuff until it works. Will update the file 
once its finished.


-- 
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: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 6:

(2 comments)

Thank you for the review!  Certainly we should communicate on our expectations 
and requirements.  I considered this original David's patch as a foundation to 
build the necessary functionality to run jepsen test against freshly built Kudu 
binaries.  You can find the result at: 
http://sandbox.jenkins.cloudera.com/view/Kudu/job/kudu-jepsen/

I decided to address some comments right away to resolve the ambiguity and 
suspense.  I'm planning to get back on the rest when I'm back from vacation.

I'm not sure what would be the best way to proceed here: merge everything into 
one patch or something else.  As you can see, I have two additional patches 
based on this:

https://gerrit.cloudera.org/#/c/5500/
https://gerrit.cloudera.org/#/c/5551/
https://gerrit.cloudera.org/#/c/5559/

I hope that answers at least some questions.

http://gerrit.cloudera.org:8080/#/c/5492/6/java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
File java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj:

Line 74:   (c/su
> So the obvious question is: why do we need to use system packages with Jeps
In my subsequent patch there are provisions to run that with binaries only.  
That was the original approach that David and I were running at our laptops in 
Docker for Mac.

I think we need two options here: (a) run against built packages (Debian only 
due to Jepsen restrictions) and (b) run against freshly built binaries.  In 
http://sandbox.jenkins.cloudera.com/view/Kudu/job/kudu-jepsen/ the latter 
approach is used.  And that will be an option.


Line 74:   (c/su
> In general, it's much easier to run tests without root access on the remote
The root access is needed to manage IP filter and other nemesis (i.e. 
failure-injection) stuff  -- that's how Jepsen does its failure injection thing 
on DB nodes.  I don't think we are about to re-do that part, at least in the 
nearest future.  Ideally, it would be nice to run everything using our 
minicluster (as Dan already mentioned somewhere), but that would result in 
invasive changes on Jepsen core code.  So, I would opt for keeping requirement 
for root access at remote nodes, so we are conformant with Jepsen 
infrastructure.

The NTP part could be removed if we add a separate provision in our Kudu source 
to ignore non-synced time -- that we will need eventually anyway (I started 
that patch, but it's not ready for review yet).  But as for now I don't see a 
problem 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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5492/6/java/pom.xml
File java/pom.xml:

Line 308: 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5492/9/build-support/jenkins/build-and-test.sh
File build-support/jenkins/build-and-test.sh:

PS9, Line 90: 
: # Set up default
How about just "echo JAVA8_HOME: $JAVA8_HOME"? That way it's more clear if it's 
unset, plus it's also clear from the output that you're testing the env 
variable with that exact name.

Though, I should ask, what exactly is this for?


http://gerrit.cloudera.org:8080/#/c/5492/9/build-support/jenkins/toolchains.xml
File build-support/jenkins/toolchains.xml:

Line 4
OK, but effect does this file have? Would be nice to explain in this comment, 
or somewhere else.


Line 11
This looks like a macOS path. What's it doing in a jenkins/... file?


Line 13
Nit: got some extra whitespace here.


http://gerrit.cloudera.org:8080/#/c/5492/6/java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
File java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj:

Line 74:   (c/su
> For expediency, mostly. However I don't think there is much to gain from ru
In general, it's much easier to run tests without root access on the remote 
nodes than with. I didn't emphasize that in my comment, but that's really what 
I'm driving at here: can we avoid using root?

Part of that is not using system packages, which obviously require root to be 
installed.

Part of that is also the ntp configuration; is it highly customized, or what 
you'd find in a typical deployment? (I asked Alexey about this on HipChat; I 
forgot you were going to take over this patch again).

I don't know how Jepsen does what it does; maybe this is just the tip of the 
iceberg and root is needed all over the place (if so, I would have liked to see 
that (important) requirement documented in the commit message). But my goal is 
to have a conversation about what's possible and what's not, and to learn about 
Jepsen's overall system requirements, which haven't been  well communicated yet.


-- 
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: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5492

to look at the new patch set (#9).

Change subject: Kudu Jepsen Tests - Initial Commit
..

Kudu Jepsen Tests - Initial Commit

This patch contains the basic jepsen tests code as it
was before KUDU-798 was resolved (i.e. as it was when it
was failing).

The clojure code is integrated into the Kudu maven build
and is compiled along with the other projects. Tests are
currently skipped as they require tests infrastructure.

Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
---
M build-support/jenkins/build-and-test.sh
A build-support/jenkins/toolchains.xml
A java/kudu-jepsen/.gitignore
A java/kudu-jepsen/pom.xml
A java/kudu-jepsen/resources/kudu.flags
A java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/table.clj
A java/kudu-jepsen/src/test/clojure/jepsen/kudu_test.clj
M java/pom.xml
12 files changed, 560 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5492/9
-- 
To view, visit http://gerrit.cloudera.org:8080/5492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[David Ribeiro Alves (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5492

to look at the new patch set (#7).

Change subject: Kudu Jepsen Tests - Initial Commit
..

Kudu Jepsen Tests - Initial Commit

This patch contains the basic jepsen tests code as it
was before KUDU-798 was resolved (i.e. as it was when it
was failing).

The clojure code is integrated into the Kudu maven build
and is compiled along with the other projects. Tests are
currently skipped as they require tests infrastructure.

Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
---
A java/kudu-jepsen/.gitignore
A java/kudu-jepsen/pom.xml
A java/kudu-jepsen/resources/kudu.flags
A java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/table.clj
A java/kudu-jepsen/src/test/clojure/jepsen/kudu_test.clj
M java/pom.xml
10 files changed, 524 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5492/7
-- 
To view, visit http://gerrit.cloudera.org:8080/5492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 6:

(11 comments)

I only skimmed the Jepsen code; mostly I was looking at how this integrates 
into the rest of the system.

http://gerrit.cloudera.org:8080/#/c/5492/6//COMMIT_MSG
Commit Message:

PS6, Line 17: WIP as this still needs some cleanup/minimal docs but
: should be reasonably ready for a first review iteration.
Should probably be updated now that this is going to be committed, right?


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

Line 1: /store
What is this for? Perhaps add a comment just before it?


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

Let's either rewrite this to be useful or remove it altogether.


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

PS6, Line 26: 
: true
: 
Why this? Add an XML comment to explain.


Line 31: 
Remove this.


Line 49: 0.1.3
I think our convention is to list all of our dependent versions in the main 
pom.xml rather than in each subproject.


Line 56: 
Why the slf4j-api exclusion here and below? Add a comment explaining.


Line 89:   true
If this is a default value, can you remove it? If not, could you add a comment 
explaining why we're using it?


http://gerrit.cloudera.org:8080/#/c/5492/6/java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
File java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj:

Line 17: ;; TODO allow to replace the binaries with locally built ones
Yes. I assume Maven is used to launch the Jepsen tests? If so, it would be nice 
if these were specified as maven properties that could be overridden.


Line 74:   (c/su
So the obvious question is: why do we need to use system packages with Jepsen? 
If we could use regular old binaries:
1) We could run Jepsen on el platforms too, and
2) I don't think we'd need to be root.


http://gerrit.cloudera.org:8080/#/c/5492/6/java/pom.xml
File java/pom.xml:

Line 308: 

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

Change subject: Kudu Jepsen Tests - Initial Commit
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5492/4/java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
File java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj:

Line 32:   (when (> (count (:masters test)) 1)
> Why only set the master addresses when there are more than one master?  Thi
As it turned out, there is a check in master_options.cc, in constructor 
MasterOptions::MasterOptions(), around line 45 -- at least 2 masters are 
required for a distributed config.  Specifying --master_addresses flag means 
using distributed/clustered master configuration.

I'll update the comment.


-- 
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Kudu Jepsen Tests - Initial Commit

[Alexey Serbin (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/5492

to look at the new patch set (#6).

Change subject: Kudu Jepsen Tests - Initial Commit
..

Kudu Jepsen Tests - Initial Commit

This patch contains the basic jepsen tests code as it
was before KUDU-798 was resolved (i.e. as it was when it
was failing).

The clojure code is integrated into the Kudu maven build
and is compiled along with the other projects. Tests are
currently skipped as they require tests infrastructure.

WIP as this still needs some cleanup/minimal docs but
should be reasonably ready for a first review iteration.

Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
---
A java/kudu-jepsen/.gitignore
A java/kudu-jepsen/README.md
A java/kudu-jepsen/pom.xml
A java/kudu-jepsen/resources/kudu.flags
A java/kudu-jepsen/src/main/clojure/jepsen/kudu.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/client.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/nemesis.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/register.clj
A java/kudu-jepsen/src/main/clojure/jepsen/kudu/table.clj
A java/kudu-jepsen/src/test/clojure/jepsen/kudu_test.clj
M java/pom.xml
11 files changed, 535 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/92/5492/6
-- 
To view, visit http://gerrit.cloudera.org:8080/5492
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I590c6e78840304b3131666c7037ff9a08dc77dea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: David Ribeiro Alves 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon