Various cleanups for Elasticsearch test #51

Merged
merged 5 commits into from Apr 17, 2015

Projects

None yet

2 participants

@dakrone
Contributor
dakrone commented Apr 7, 2015

This change makes four changes:

  • Adding a logging configuration

In order to actually determine the root cause of issues, more verbose logging is needed. This defaults to more verbose logging for Elasticsearch and adds the ability to change it from Jepsen in the future (instead of manually by hand).

  • Moving nuke! to before tests

Without this, Jepsen deletes all traces of itself after running, which makes debugging much more difficult (no logs and no data left).

  • Use more reasonable settings for scroll

An optional change, but I figured I would make this change anyway.

  • Wait for index to become green after creation

This is a key part of testing Elasticsearch, and clients should always do this when creating indices.


Note that I was able to reproduce the failures in elastic/elasticsearch#10426 (about half of the time) without these changes, however after the change which waits for green after index creation, I am no longer able to reproduce data loss with the create-pause test (still evaluating the other tests).

dakrone added some commits Apr 7, 2015
@dakrone dakrone Add logging configuration
This increases the default logging to DEBUG, and sets TRACE logging for
gateway and discovery packages.
aa66375
@dakrone dakrone Move `nuke!` to before the test starts
This allows someone to collect the ES logs and data *after* a test run.
Otherwise the logs and data is removed and ES is stopped, making further
debugging impossible.
efc15e4
@dakrone dakrone Wait for index to become green after creation
After an index is created, clients should always wait for the index to
be fully created (the request returns immediately) before starting the
test.
22a50b2
@dakrone dakrone Use more reasonable settings for scroll
No need for the `query_then_fetch` setting, use ten seconds instead of
one minute, and a more reasonable size of 20 rather than 2.
b1cb921
@dakrone
Contributor
dakrone commented Apr 8, 2015

@aphyr also, how would you feel about me replacing Elastisch with vanilla clj-http? Elastisch uses clj-http internally anyway and it would reduce the number of moving parts in this test. I'm happy to submit another PR if you are interested.

@aphyr aphyr and 1 other commented on an outdated diff Apr 15, 2015
elasticsearch/src/elasticsearch/core.clj
(try
(esi/create client index-name
:mappings {"number" {:properties
{:num {:type "integer"
:store "yes"}}}}
- :settings {"index" {"refresh_interval" "1"}})
- (catch clojure.lang.ExceptionInfo e
- ; Is this seriously how you're supposed to do idempotent
- ; index creation? I've gotta be doing this wrong.
- (let [err (http-error e)]
- (when-not (re-find #"IndexAlreadyExistsException" err)
- (throw (RuntimeException. err))))))
-
+ :settings {"index" {"refresh_interval" "1s"}})
+ (catch Throwable t))
@aphyr
aphyr Apr 15, 2015 Collaborator

I'd really prefer to know about connection errors etc that happen here; the only reason it's appropriate to noop is if the index already exists.

@dakrone
dakrone Apr 16, 2015 Contributor

Sure, I will remove this to only handle IndexAlreadyExistsException as you previously did. I do think using basic clj-http would be easier, as it'd allow you to use:

(try+
  ...
  (catch [:status 400]
    ;; ignore
@aphyr aphyr commented on the diff Apr 15, 2015
elasticsearch/src/elasticsearch/core.clj
@@ -220,9 +221,8 @@
(esi/flush client index-name)
(assoc op :type :ok
:value (->> (esd/search client index-name "number"
- :search_type "query_then_fetch"
- :scroll "1m"
- :size 2)
+ :scroll "10s"
+ :size 20)
@aphyr
aphyr Apr 15, 2015 Collaborator

Yes, that's much more sensible, thank you. :)

@aphyr aphyr commented on an outdated diff Apr 15, 2015
elasticsearch/src/elasticsearch/core.clj
(try
(esi/create client index-name
:mappings {mapping-type {:properties {}}})
- (catch clojure.lang.ExceptionInfo e
- ; Is this seriously how you're supposed to do idempotent
- ; index creation? I've gotta be doing this wrong.
- (let [err (http-error e)]
- (when-not (re-find #"IndexAlreadyExistsException" err)
- (throw (RuntimeException. err))))))
+ (catch Throwable t))
@aphyr
aphyr Apr 15, 2015 Collaborator

Same here; I want to be conservative about which errors I'll ignore, haha. :)

@aphyr aphyr and 1 other commented on an outdated diff Apr 15, 2015
elasticsearch/src/elasticsearch/core.clj
(try
(esi/create client index-name
:mappings {"number" {:properties
{:num {:type "integer"
:store "yes"}}}}
- :settings {"index" {"refresh_interval" "1"}})
- (catch clojure.lang.ExceptionInfo e
- ; Is this seriously how you're supposed to do idempotent
- ; index creation? I've gotta be doing this wrong.
- (let [err (http-error e)]
- (when-not (re-find #"IndexAlreadyExistsException" err)
- (throw (RuntimeException. err))))))
-
+ :settings {"index" {"refresh_interval" "1s"}})
+ (catch Throwable t))
+ (esa/cluster-health client
+ {:index [index-name] :level "indices"
+ :wait_for_status "green"
+ :wait_for_nodes "5"})
@aphyr
aphyr Apr 15, 2015 Collaborator

Should "5" be a string instead of a number? Either way, we should use (count (:nodes test)).

@dakrone
dakrone Apr 16, 2015 Contributor

It can be either a string or a number, I will change this to be (count (:nodes test)) as you said.

@dakrone dakrone Catch only the `IndexAlreadyExistsException`, use dynamic node count
Instead of catching any throwable during index creation, catch a
specific exception and ensure it was only because the index already
existed.

Additionally, this changes the hardcoded node count of 5 to
`(count (:nodes test))` so a dynamic number of nodes can be used.
4f3a27f
@dakrone
Contributor
dakrone commented Apr 17, 2015

Pushed another commit addressing your feedback, thanks for taking a look!

@aphyr
Collaborator
aphyr commented Apr 17, 2015

Excellent, thanks @dakrone :)

@aphyr aphyr merged commit 8bbd973 into jepsen-io:master Apr 17, 2015
@aphyr
Collaborator
aphyr commented Apr 28, 2015

Do these tests run for you? Elasticsearch doesn't even start on my nodes any more; times out waiting for cluster recovery.

@dakrone
Contributor
dakrone commented Apr 28, 2015

@aphyr I just double-checked this and the tests are still running for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment