Rewrite client to support high volume of topics #234
Conversation
|
It looks like its a breaking change as current version allows GroupConsumer to subscribe to multiple topics (via |
|
@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? |
|
@mnorkin Well actually it also breaks the |
|
@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. |
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 |
|
I'm adding more tests |
…ockerized kafka testkit
|
@oleksiyk updated the description, build is green 🎉 |
lib/group_consumer.js
Outdated
| @@ -196,7 +201,7 @@ GroupConsumer.prototype._fullRejoin = function () { | |||
| }); | |||
| }) | |||
| .catch(function (err) { | |||
| self.client.error('Full rejoin attempt failed:', err); | |||
| self.client.error('Full rejoin attempt failed:', JSON.stringify(err)); | |||
There was a problem hiding this comment.
@mnorkin why do you stringily error here?
|
@oleksiyk any updates? |
|
@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. |
| buffer = self.protocol.write().ListGroupsRequest({ | ||
| correlationId: correlationId, | ||
| clientId: self.options.clientId | ||
| }).result; | ||
|
|
||
| return Promise.map(_.values(self.brokerConnections), function (connection) { | ||
| return Promise.map(_.values(self.initialBrokers), function (connection) { |
There was a problem hiding this comment.
@mnorkin Not sure if it can happen that brokerConnection won't include all of the initialBrokers? I'm asking because ListGroupsrequest must be sent to ALL brokers, or you won't receive all groups.
There was a problem hiding this comment.
@oleksiyk How I understood, this command is executed only by groupAdmin and it does not contain any information about the topics, so no topology query here, hence only initialBrokers is present. It works if you know the the topology before hand, it does not work in cause you have more brokers, than initially defined.
However I believe this implementation is better than having all the topology read from the all brokers and freeze your application in case you have 100k topics
|
@mnorkin Can you please make new PR, I accidentally merged this one while thinking I'm looking at other project page. Sorry about this. |
|
And can you please revert the ability to run tests locally, without Doker. |
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.
npm run test