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

Make consumer.subscribe accept an array of topics #1313

Merged
merged 3 commits into from
Apr 8, 2022

Conversation

benvan
Copy link
Contributor

@benvan benvan commented Mar 12, 2022

Feature: consumer.subscribe now accepts String, String[] or RegExp

Motivation: At work, many of our consumers subscribe to a handful of related topics. Reducing the number of consumer.subscribe calls in the proposed manner significantly reduces our consumer startup time (50%)

Justification: Using RegExp is already essentially supplying a string array, just indirectly. If you know the topics you want to subscribe to, this avoids you needing to craft a regex which is built from those topic names (and the cost of the subsequent call to the broker to download the topic list)

Note: This PR does not have an associated issue - if this is a necessary requisite then apologies in advance and I'm happy to oblige

@benvan benvan force-pushed the feat-subscribe-to-topic-array branch from a606c44 to 2ef4ebb Compare March 12, 2022 18:07
@Nevon
Copy link
Collaborator

Nevon commented Mar 16, 2022

Makes sense. I would just like to deprecate the topic argument and instead have topics: string[] | RegExp. We should still accept the topic argument for backwards compatibility, but throw an error if both topic and topics are provided. So the type of ConsumerSubscribeTopic becomes { fromBeginning?: boolean } & ({ topic: string | RegExp; } | { topics: string[] | RegExp; }) Feels weird to have an argument called topic that takes an array.

But then the question becomes, should we also accept multiple regular expressions for topics? You can define one regex to match all the desired topics, but maybe it's easier to have multiple regexes. So it'd be { fromBeginning?: boolean } & ({ topic: string | RegExp; } | { topics: string[] | RegExp[]; }) in that case.

@benvan
Copy link
Contributor Author

benvan commented Mar 23, 2022

Definitely agree re. topics vs topic. I'll submit an update, hopefully this weekend.

Regarding the plural regex/string question - I'll make it take multiple regexes unless you have any objections

EDIT: By which I mean { topics: (string | RegExp)[] }. Example usage would be { topics: ["topic-A", /special-topic-[0-9]+/] }

@Nevon
Copy link
Collaborator

Nevon commented Mar 23, 2022

Originally I was thinking string[] | RegExp[] because mixed-type arrays always make me feel like I must be doing something wrong, but in this case I agree it makes sense to do (string | RegExp)[]

@Nevon
Copy link
Collaborator

Nevon commented Apr 8, 2022

I went ahead and updated this with the interface we discussed.

@Nevon Nevon merged commit 77d2bf8 into tulios:master Apr 8, 2022
@benvan
Copy link
Contributor Author

benvan commented Apr 10, 2022

Thank you @Nevon!

@Nevon Nevon mentioned this pull request May 2, 2022
10 tasks
Nevon added a commit that referenced this pull request May 2, 2022
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.

None yet

2 participants