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

GOJ-87311 Add producing support in Ziggurat via Kafka Producer #50

Closed
wants to merge 4 commits into from

Conversation

mjayprateek
Copy link
Contributor

No description provided.

Copy link
Member

@kartik7153 kartik7153 left a comment

Choose a reason for hiding this comment

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

@mjayprateek Right now, Ziggurat can read from multiple Kafka topics and from different Kafka servers.
Can we have something producing support as well? Like publishing to different Kafka servers.
Currently, you are mounting the Kafka producer with one Kafka server.

@kartik7153
Copy link
Member

Also, Raise the PR against 2.x branch.

:enabled [true :bool]}}}}}
:producer {:default {:bootstrap-servers "localhost:9092"
Copy link
Member

Choose a reason for hiding this comment

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

What is this default key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kartik7153 default just refers to the default KafkaProducer config. Let me know if this needs change.

@@ -113,7 +113,7 @@
topic-pattern (Pattern/compile origin-topic)]
(.addStateStore builder (store-supplier-builder))
(->> (.stream builder topic-pattern)
(transform-values topic-entity-name oldest-processed-message-in-s)
(transform-values topic-entity-name 1)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By mistake. Removed.

(deftest send-data-with-topic-and-value-test
(let [topic "test-topic"
data "Hello World!!!"]
(let [future (producer/send topic data)]

Choose a reason for hiding this comment

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

can this be in the last let only? like:

    (let [  topic "test-topic"
              data "Hello World!!!"
              future (producer/send topic data)]

also, what happens if the topic is nil? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@riteeks You're right. But, I think these two lets logically divide the code in two parts:

  1. First let setups the data for calling send()
  2. Second let simply acts on the result of calling send()

Mixing these two, IMHO, will lead to a less readable code.

send in ziggurat is just a wrapper around KafkaProducer#send. Thus, whatever arguments are passed will just be passed "as they are" to KafkaProducer. That class will be responsible for error handling and exceptions. Ziggurat's code is just a thin layer in-front
of KafkaProducer.

Choose a reason for hiding this comment

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

Mixing these two, IMHO, will lead to a less readable code.

make sense.

send in ziggurat is just a wrapper around KafkaProducer#send. Thus, whatever arguments are passed will just be passed "as they are" to KafkaProducer. That class will be responsible for error handling and exceptions. Ziggurat's code is just a thin layer in-front
of KafkaProducer.

for this, I was just thinking of having a test for bad-path also for the ziggurat's send function. But I am not very sure how much value will this add.

@mjayprateek
Copy link
Contributor Author

@mjayprateek Right now, Ziggurat can read from multiple Kafka topics and from different Kafka servers.
Can we have something producing support as well? Like publishing to different Kafka servers.
Currently, you are mounting the Kafka producer with one Kafka server.

@kartik7153 I agree with you on this. The only glitch is that this will increase the scope of this story. Prima facie, supporting multiple producers needs more thought and some discussion. I'll create a GitHub issue for that and we can take that forward in the next sprint.

Let me know if you agree on this.

Removing unintentional changes from streams.clj
@mjayprateek
Copy link
Contributor Author

Closing this PR as the the changes were meant to be raised against branch 2.x.
New PR is a #55

@mjayprateek mjayprateek closed this Jun 6, 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.

None yet

3 participants