Rewrite client to support high volume of topics

oleksiyk / kafka

Apache Kafka 0.9 client for Node

MIT License

297 stars 85 forks source link

Rewrite client to support high volume of topics #234

Closed mnorkin closed 5 years ago

mnorkin commented 5 years ago

Implement high performant metadata read from kafka cluster.

Previous implementation looped over all the topics, which it received from kafka cluster. Current implementation fetches only metadata by topic, so no loop over all topics is required.

Rewrote couple of tests to reduce dependencies between tests
Added more test to prove single instance multi topic publish/ consume/ group consume
Wrote a kafka testkit to dynamically create topics. Now there is no need to download, run kafka and create topics for tests to work - just npm run test
No API changes to the client

oleksiyk commented 5 years ago

It looks like its a breaking change as current version allows GroupConsumer to subscribe to multiple topics (via subscriptions array).

mnorkin commented 5 years ago

@oleksiyk sorry, was not sure about that than I was refactoring. Will update tomorrow.

Should I add test for GroupConsumer to check for multiple topics?

oleksiyk commented 5 years ago

@mnorkin Well actually it also breaks the Producer too, which allows to send messages to any topic.

mnorkin commented 5 years ago

@oleksiyk I've rewritten the client to not break API. I'll make some performance comparisons for high number of topics to provide some evidence why this PR is important.

oleksiyk commented 5 years ago

I'll make some performance comparisons for high number of topics to provide some evidence why this PR is important.

I do believe there is significant difference. However there must be tests added that verify that this PR really supports producing and consuming from multiple topics within the same instance of Producer, SimpleConsumer or GroupConsumer. Otherwise I won't be able to merge it.

mnorkin commented 5 years ago

I'm adding more tests

mnorkin commented 5 years ago

@oleksiyk updated the description, build is green 🎉

mnorkin commented 5 years ago

@oleksiyk any updates?

oleksiyk commented 5 years ago

@mnorkin Give me some time to check and try it, it is a big update and I don't want to break any existing code relying on this library.

oleksiyk commented 5 years ago

@mnorkin Can you please make new PR, I accidentally merged this one while thinking I'm looking at other project page. Sorry about this.

oleksiyk commented 5 years ago

And can you please revert the ability to run tests locally, without Doker.