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

Word Count Modernization #191

Merged
merged 4 commits into from
Sep 20, 2019
Merged

Word Count Modernization #191

merged 4 commits into from
Sep 20, 2019

Conversation

creese
Copy link
Contributor

@creese creese commented Sep 13, 2019

This PR updates the "Word Count" example.

Highlights:

  • Removes the ad hoc start/stop functions in favor of James Reeves's Integrant.
  • Adds a dev alias.
  • Adds separate Integrant configs for dev and prod.
  • Adds a topic metadata "fake" for use in development.
  • Adds a namespace with REPL helpers to create topics, and produce and consume records.
  • Copies some parsing logic from the Topology Grapher.

When writing code, you can treat the topic metadata fake just like a map. When used with a "getter", it returns the topic metadata for the name supplied, EDN serdes for the key and value, and a partition count of one.

(def topic-metadata (new-fake-topic-metadata))
(:foo topic-metadata)
;; => {:topic-name "foo", 
       :partition-count 1,
       :replication-factor 1,
       :key-serde  #object[org.apache.kafka.common.serialization.Serdes ... ],
       :value-serde #object[org.apache.kafka.common.serialization.Serdes ... ]}

When in development, the topics are inferred from the topology. The dev workflow is largely unchanged. Simply jack in and type:

(reset)
;; => :reloading (word-count user)
;; => :resumed
(publish (:input topic-metadata) nil "all streams lead to kafka")
(publish (:input topic-metadata) nil "hello kafka streams")
(get-keyvals (:output topic-metadata))
;; => (["all" 1] ["streams" 1] ["lead" 1] ["to" 1] ["kafka" 1] ["hello" 1] ["kafka" 2] ["streams" 2])

The user namespace is part of the dev alias and is not loaded automatically. To apply the dev alias, add the following to your Emacs config:

(setq cider-clojure-cli-global-options "-A:server:client:dev")

or type C-u -A:server:client:dev when jacking in.

@creese creese requested a review from a team as a code owner September 13, 2019 23:02
(is (= 1 (word-count journal "understand")))
(is (= 2 (word-count journal "i")))
(is (= 3 (word-count journal "to"))))))))
;; (ns word-count-test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented out this namespace while development. I plan to put this back.

@codecov
Copy link

codecov bot commented Sep 13, 2019

Codecov Report

Merging #191 into master will decrease coverage by 2.68%.
The diff coverage is 18.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   81.43%   78.74%   -2.69%     
==========================================
  Files          40       41       +1     
  Lines        2391     2498     +107     
  Branches      149      149              
==========================================
+ Hits         1947     1967      +20     
- Misses        295      382      +87     
  Partials      149      149
Impacted Files Coverage Δ
src/jackdaw/streams/describe.clj 18.69% <18.69%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f404699...8c9e685. Read the comment docs.

{:topic-name (name key)
:partition-count 1
:replication-factor 1
:key-serde (js/edn-serde)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we should use a string serde for the key to make it easier to join with other topics. See https://github.com/FundingCircle/jackdaw-repl/commit/1ddd902e3ec325c862bdb2ef06f01050255b665e

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Keep in mind this is just for prototyping. We shouldn't use this or EDN keys in production.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking we could add a new arity that takes an argument so the user can set their own default. Would that address your use case?

(str/join "|"))
")")))))

(defmethod ig/halt-key! :app [_ {:keys [streams-config streams-app]}]
Copy link
Contributor

Choose a reason for hiding this comment

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

I started with that, and then separated out the cleanup or resources because sometimes you just want to stop the app but not delete everything so you can inspect the topics and state stores. https://github.com/FundingCircle/jackdaw-repl/commit/0ba5f6f70ab2a08f8f4d11819c5540d3e093574c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like you'd want a full reset to be able to start clean. Could we halt just the app with a different config structure?

(assoc [this key val]
this))

(defn new-fake-topic-metadata []
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just use a single instance of FakeTopicMetadata, so we can drop this function and update topic-metadata to be:

(def topic-metadata (FakeTopicMetadata.))

Then we can just use topic-metadata where new-fake-topic-metadata is currently used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could do. Does this still make sense if we add an arity to allow the user to set their own default?



(defmethod ig/init-key :streams-config [_ streams-config]
(let [bootstrap-servers (or (System/getenv "BOOTSTRAP_SERVERS")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move this logic to where the config map is instantiated, and get rid of this init-key method

(let [client-config (assoc consumer-config
"group.id"
(str (java.util.UUID/randomUUID)))]
(with-open [client (jc/subscribed-consumer client-config [topic-config])]
Copy link
Contributor

Choose a reason for hiding this comment

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

When using this function from the repl, I've always needed to call seek-to-beginning-eager on the consumer before using it to consume messages. This may just be my setup though 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This config sets a random UUID for the consumer group. It will always start from the beginning. If we were to stop doing that, I think we would need to do as you suggest. I'm not sure which is preferable.

;; Evaluate the form:
(let [text-input (slurp (io/resource "metamorphosis.txt"))
values (str/split text-input #"\n")]
(doseq [v values]
(publish (:input topic-metadata) nil v)
(info v))
(info "The End"))
(println v))
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nice not to print every line, but some other (smaller) progress indicator. I use a fast terminal and it took a while - in an emacs buffer I imagine its a bit painful ...

99-not-out
99-not-out previously approved these changes Sep 20, 2019
Copy link
Contributor

@99-not-out 99-not-out left a comment

Choose a reason for hiding this comment

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

Nice - works for me with the tweak for clj -A:dev in the CLI based instructions.

@creese creese merged commit 9b5bbd0 into master Sep 20, 2019
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.

4 participants