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

Migrate from spring-kafka to native Apache Kafka client #10379

Merged
merged 8 commits into from
Sep 22, 2019

Conversation

ctamisier
Copy link
Contributor

PR candidate for fixing #9287

@ctamisier ctamisier changed the title Migrate from spring-cloud-stream to native Apache Kafka client Migrate from spring-kafka to native Apache Kafka client Sep 10, 2019
@ctamisier ctamisier marked this pull request as ready for review September 10, 2019 13:02
@pascalgrimaud pascalgrimaud self-assigned this Sep 10, 2019
<%_ if (messageBroker === 'kafka') { _%>
kafka:
consumer:
key.deserializer: org.apache.kafka.common.serialization.StringDeserializer
Copy link
Member

Choose a reason for hiding this comment

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

As you updated the properties here, you need to do the same for :

  • local app.yml
  • docker-compose sub gen
  • kubernetes sub gen
  • openshift sub gen
  • etc

@pascalgrimaud
Copy link
Member

Just tested and it works ! Really great job @ctamisier
I added some minor comments.

The most important missing part is the properties in YAML files for docker-compose, k8s etc.

@ctamisier
Copy link
Contributor Author

ctamisier commented Sep 13, 2019

Thanks for the review !
I've pushed some changes and I need to update the Gradle part as well.

@pascalgrimaud
Copy link
Member

@ctamisier : don't hesitate to ping me as soon as you update this PR, so I can have a look again. If you don't have time, just tell me, I'll help you to update this, as I'd like to have this for the next release and start contribution on the entity part + Kafka

@ctamisier
Copy link
Contributor Author

Since your last review I applied the following changes:

  • Add Gradle support (build.gradle.ejs)
  • Add <logger name="org.apache.kafka" level="INFO"/> (org.apache is in WARN) (To be discussed eventually)
  • Remove <logger name="kafka" level="WARN"/> and <logger name="org.I0Itec" level="WARN"/>

I'm not really good about docker compose file definition... I tried to make docker-compose -f src/main/docker/app.yml up working but the app can't connect to kafka.

I don't know what generator-jhipster/test/templates/compose/09-kafka/src/main/docker/kafka.yml is for, so I didn't modify it. Should it be a "generation" of generator-jhipster/generators/server/templates/src/main/docker/kafka.yml.ejs ? (same with app.yml and app.yml.ejs)

We still need to adjust app.yml.ejs (I've an issue now because I use depends_on in kafka.yml.ejs).

Your help @pascalgrimaud is really welcome, I'm kind of learning docker-compose right now and I might not have the "Best Practices" to apply on this part.

@pascalgrimaud
Copy link
Member

ok @ctamisier
I'll start the work on this PR tonight or tomorrow and will probably commit directly to your branch

@pascalgrimaud
Copy link
Member

Just done the review and the tests.
Everything works fine ! So going to merge this as soon as the CI is green.

good job @ctamisier and @clement26695 !

@ctamisier : plz, don't rebase / force push, otherwise, we will lost our work here :)

@ctamisier ctamisier deleted the kafka branch October 12, 2019 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants