Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for Consul tests #23

Merged
merged 13 commits into from Jan 25, 2015
Merged

Adding support for Consul tests #23

merged 13 commits into from Jan 25, 2015

Conversation

armon
Copy link
Contributor

@armon armon commented Apr 17, 2014

This is probably a very newb PR so please forgive. I've cargo-culted most of this to make it work based on the etcd work. The one thing is it seems Jepsen does value based CAS while Consul does index based CAS. I've worked around to first do a read to get the index and then do the CAS operation. Not sure if there is a way to improve this by passing the index from a read into the CAS operation. Thanks!

@jpfuentes2
Copy link
Contributor

I was still looking to clean this up if you're interested. One of the things I wanted to remove was the etcd workarounds that accounts for the startup failure bug. Also, I'm looking to finish up a Clojure-consul client soon enough.

I think we can handle the index-based CAS by passing it to the op since it's just a hash-map:

Roughly:

:read  (try (let [response (consul-get client)
                  value (parse-value response)
                  idx (parse-index response)]
              (assoc op :type :ok :value value :index idx))

:cas   (try
           (let [[value value'] (:value op)
                 ok?            (consul-cas client
                                            (:index op)
                                            (json/generate-string value)
                                            (json/generate-string value'))]

@aphyr
Copy link
Collaborator

aphyr commented Apr 18, 2014

Sorry for the delay; came into work and found a fire. Lemme see.

true
(catch RuntimeException _ false)))

(defn start-consul!
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have init scripts for Consul, it might be more concise to install and call them using service consul start.

@armon
Copy link
Contributor Author

armon commented Apr 18, 2014

@aphyr Thanks for the feedback. Been a long day, will try to cleanup the PR tomorrow and work with @jpfuentes2 on it.

@jpfuentes2
Copy link
Contributor

Okay, I made a lot of changes to clean up some of the cruft and copy-pasta from the etcd example. Also improved the style to be a better Clojure citizen.

I welcome further review whenever you have a chance. Thanks!

(reify db/DB
(setup! [this test node]
; bring down any lingering processes
(db/teardown! this test node)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Teardown actually gets called for you prior to setup!, so you can elide this too. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I see that now:

(defn cycle!
  "Tries to tear down, then set up, the given DB."
  [db test node]
  (try (teardown! db test node)
       (catch RuntimeException _))
  (setup! db test node))

@aphyr
Copy link
Collaborator

aphyr commented Jan 23, 2015

OK, all this looks generally good to me! Sorry about the insane wait, I'm whittling down the PR backlog this month. :)

@jpfuentes2
Copy link
Contributor

Sounds good to me. Do you want me to try my hand at resolving merge conflicts?

@aphyr aphyr merged commit 427df28 into jepsen-io:master Jan 25, 2015
@aphyr
Copy link
Collaborator

aphyr commented Jan 25, 2015

Got it merged, porting your stuff forward to the new code structure now.

@aphyr
Copy link
Collaborator

aphyr commented Jan 25, 2015

OK, so I hooked up the dependency machinery and all; you can run the tests with lein with-profile +consul test jepsen.system.consul-test, but they crash with /var/log/consul.log: Permission denied; probably just an issue in the setup permissions but I'm not gonna dig into it too deep right now.

aphyr pushed a commit that referenced this pull request Feb 13, 2019
aphyr pushed a commit that referenced this pull request Apr 1, 2019
def- pushed a commit to def-/jepsen that referenced this pull request May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants