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

Kafka singleton consumer #947

Merged
merged 4 commits into from Sep 25, 2019
Merged

Conversation

dirk39
Copy link
Contributor

@dirk39 dirk39 commented Sep 11, 2019

Hi, introduce a rdKafkaConsumers collection to avoid the creation of multiple consumers for the same queue. This PR solve the issue #894 and #893 (I've tried the solution on this project and it works)

@Steveb-p
Copy link
Contributor

I was wondering if it's the responsibility of enqueue to keep consumers cached. Theoretically one should never want to create a second consumer to the same broker, but I could see a potential use-case when one creates two consumers with different group.id's. You won't be able to do so if you're only checking for Kafka topic name.

I think this caching should be done in adapter instead. WDYT @dirk39 ?

@dirk39
Copy link
Contributor Author

dirk39 commented Sep 11, 2019

@Steveb-p caching in adapter could be a good solution, but taking in examples the sroze implementation, he uses the same class (QueueInteropTransport) to execute the ack method and this is valid for any context. Kafka is sensible about consumers because he associate any consumer to a specific partition so i think is important to return same consumer if already instantiate.

@Steveb-p
Copy link
Contributor

@dirk39 I was thinking about this and eventually came to the conclusion that there really is no good approach other than the one you provided. We can't really cache consumers in QueueInterop (I mean we could, but imo it should remain more abstract than the implementation, right?) so caching them in enqueue is the next best thing.

Therefore, I'll review your code and see if there is anything that catches my attention. I'll request a review from @makasim but will merge this if there is no response from him in a couple of days if it's ok with you?

@Steveb-p Steveb-p self-assigned this Sep 11, 2019
@Steveb-p Steveb-p added the bug label Sep 11, 2019
@makasim
Copy link
Member

makasim commented Sep 12, 2019

good to me. One note: clean cached consumers on context close.

@dirk39
Copy link
Contributor Author

dirk39 commented Sep 12, 2019

Ok. @Steveb-p @makasim do you believe that could be useful to discriminate consumers by group.id and queueName or can we left just queueName?

@Steveb-p
Copy link
Contributor

Ok. @Steveb-p @makasim do you believe that could be useful to discriminate consumers by group.id and queueName or can we left just queueName?

I believe using multiple consumers attached to the same topic with different group.id's use-case is rather rare. I think we can leave it as is and come back to it if it's troublesome for anyone.

Theoretically in newer versions of librdkafka group.id is no longer necessary and can be used to force reading of a topic without storing offset, but that's another story.

@dirk39
Copy link
Contributor Author

dirk39 commented Sep 23, 2019

Hi @Steveb-p , do you have an idea of when do you expect to merge the PR?

@Steveb-p
Copy link
Contributor

Hi @Steveb-p , do you have an idea of when do you expect to merge the PR?

I'm only waiting for confirmation from @makasim.

@Steveb-p
Copy link
Contributor

@makasim ?

@makasim
Copy link
Member

makasim commented Sep 25, 2019

There are two properties kafkaConsumers and rdkafkaConsumers. I could not spot a difference.

@Steveb-p
Copy link
Contributor

There are two properties kafkaConsumers and rdkafkaConsumers. I could not spot a difference.

@makasim kafkaConsumers keeps references to enqueue-based classes wrapping rdkafka consumers, while rdkafkaConsumers are rdkafka consumers without those wrappers. Wrappers do not allow direct access to internal consumers, so that's why they are separate.

Otherwise we should introduce a getter method to wrappers (which is fine and probably should be prefered?)

@makasim
Copy link
Member

makasim commented Sep 25, 2019

Otherwise we should introduce a getter method to wrappers (which is fine and probably should be prefered?)

It could be done as a separate PR

@makasim makasim merged commit 599ed87 into php-enqueue:master Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants