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

Update to remove old kafka modules #154

Merged
merged 9 commits into from
Aug 17, 2020
Merged

Conversation

rob3000
Copy link
Member

@rob3000 rob3000 commented Jul 31, 2020

Removing old node modules and use kafkaJS as primary source and removed some old config.

Suggestions/comments welcome 😄

Next i'd like to update some of the documentation thoughts on using something like: https://docusaurus.io/

@rob3000 rob3000 force-pushed the master branch 2 times, most recently from 4571439 to 641c169 Compare July 31, 2020 13:08
@krystianity
Copy link
Member

Looking good @rob3000 I am on the road currently, going to have a look at this on the weekend.

Copy link
Member

@krystianity krystianity left a comment

Choose a reason for hiding this comment

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

Not really a change request, more like suggestions and opinions ;)

lib/kafkajs/JSConsumer.js Outdated Show resolved Hide resolved
yarn.lock Show resolved Hide resolved
lib/Sinek.js Show resolved Hide resolved
@rob3000
Copy link
Member Author

rob3000 commented Aug 10, 2020

@krystianity updated based on the comments you made. I've also converted to typescript. Currently admin client is failing but feel free to review :)

@rob3000 rob3000 marked this pull request as ready for review August 11, 2020 05:25
@rob3000
Copy link
Member Author

rob3000 commented Aug 11, 2020

@krystianity This is now ready for review! 🥳

@rob3000 rob3000 requested a review from krystianity August 11, 2020 05:51
…t client as abstract so the consumer/producer is forced to set the client
@krystianity
Copy link
Member

Awesome, will review tonight - you are really going full speed here 👍 ❤️

index.d.ts Outdated Show resolved Hide resolved
src/lib/kafkajs/index.ts Outdated Show resolved Hide resolved
@krystianity
Copy link
Member

looking good so far, however the amount of changes is quite large so its quite difficult to approve this with 100% confidence, given the major changes we are doing anyway, we will probably have to run some in the fields tests for lag status (health and analytics) - testing this with kafka-streams will probably also be a good validation of the changes.

@rob3000
Copy link
Member Author

rob3000 commented Aug 13, 2020

agreed, there are lots of changes. i guess if we merge to master we can test kafka streams from the sinek's master branch which would give us better confidence?

@krystianity
Copy link
Member

@rob3000 didnt release you were waiting for my approval to merge this, lgtm ;)

@rob3000 rob3000 merged commit b789352 into nodefluent:master Aug 17, 2020
@rob3000 rob3000 mentioned this pull request Aug 18, 2020
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.

2 participants