-
Notifications
You must be signed in to change notification settings - Fork 80
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
Add Test Machine to Jackdaw #28
Conversation
9fb39b1
to
fa219b4
Compare
* Test machien run halts on first failed step * Removed the NS from the keywords for test machine commands * Made the logging stop splatting everything up the console * If a test machine command is not recognoised, return a nicer error * Added a pretty printing equivalent of the :println command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. We should confirm aviso works with Clojure 1.10 style stack traces and after we upgrade to 1.10. there is still value there.
@@ -1,7 +1,9 @@ | |||
(defproject fundingcircle/jackdaw "_" | |||
:description "A Clojure library for the Apache Kafka distributed streaming platform." | |||
|
|||
:dependencies [[clj-time "0.13.0"] | |||
:dependencies [[aleph "0.4.6"] | |||
[clj-time "0.13.0"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere, we have standardized on clojure.java-time
. This is not a blocker.
:dependencies [[clj-time "0.13.0"] | ||
:dependencies [[aleph "0.4.6"] | ||
[clj-time "0.13.0"] | ||
[org.clojure/core.async "0.4.474"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The combination of aleph
and core.async
is an interesting choice. Would it be possible refactor towards one of these (with the possible addition of manifold
)? This is not a blocker. I'm just wondering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I think that makes sense. I ended up introducing aleph to support asynchronous http requests initiated by the rest-proxy transport.
But the go-loop
in the test-machine should be relatively portable to aleph's d/loop
and it shouldn't change the external contract to do so.
@@ -66,11 +69,11 @@ | |||
|
|||
:test | |||
{:resource-paths ["test/resources"] | |||
:dependencies [[arohner/wait-for "1.0.2"] | |||
:injections [(require 'io.aviso.logging.setup)] | |||
:dependencies [[io.aviso/logging "0.3.1"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this compatible with Clojure 1.10 style stack traces? Do we use this for more than pretty printing of exceptions?
;; the options map `opts`. | ||
;; - Further, the `opts` map can contain an explciit `:key` and/or | ||
;; `:partition`, which if set will provide the values to use | ||
;; If both specified, `opts` values will override values in the topic map. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a doc-string.
`consumer` should be a kafka consumer | ||
`topic-config` should be a sequence of topic-metadata maps" | ||
[consumer topic-config] | ||
(kafka/subscribe consumer topic-config)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is subscribe
meant to be part of a public API? Why not just use jackdaw.client/subscribe
?
(doseq [assigned-partition assigned-partitions] | ||
;; This forces the seek to happen now | ||
(.position consumer assigned-partition)) | ||
consumer)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks similar to jackdaw.client/seek-to-end-eager
. Can we use that instead?
https://github.com/FundingCircle/jackdaw/blob/master/src/jackdaw/client.clj#L218
[kafka-config topic-collection] | ||
(-> (kafka/consumer kafka-config byte-array-serde) | ||
(subscribe topic-collection) | ||
(seek-to-end))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is similar to jackdaw.client/subscribed-consumer
:
https://github.com/FundingCircle/jackdaw/blob/master/src/jackdaw/client.clj#L141
[clojure.test :refer :all] | ||
[jackdaw.serdes.avro.schema-registry :as reg] | ||
[jackdaw.streams :as k] | ||
[jackdaw.test :as jd.test] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere, we've used the equivalent of '[jackdaw.test :as jt]
. I've also seen '[jackdaw.test :as j.test]
. We should standardize on one of these three.
Moving the work from @cddr into Jackdaw (all credit to @cddr).
Adds machinery to Jackdaw for running black box tests against a kafka application, either directly against the brokers or via the confluent Rest Proxy API. Allows generation of a test which can then (without modification) be run locally, in CI or against a remote environment.
Documentation will be added soon!