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

feat(sdk): Multi-topic messagebus subscriptions #614

Closed

Conversation

AlexCuse
Copy link
Contributor

Add an intermediate 'collector' channel to messagebus trigger to
aggregate messages from multiple topics for processing.

Signed-off-by: Alex Ullrich alex.ullrich@gmail.com

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number:
#596

What is the new behavior?

Allow multi-topic messagebus subscription.

Does this PR introduce a breaking change?

  • Yes
  • No

BindingInfo.SubscribeTopic was renamed to BindingInfo.SubscribeTopics - config files will need to be updated accordingly

Are there any new imports or modules? If so, what are they used for and why?

Are there any specific instructions or things that should be known prior to reviewing?

Other information

@AlexCuse AlexCuse force-pushed the messagebus-multiple-subscribe branch from 39fd6b1 to 53ceb16 Compare December 23, 2020 14:42
@codecov-io
Copy link

codecov-io commented Dec 23, 2020

Codecov Report

Merging #614 (165dc69) into master (fb6bb54) will increase coverage by 0.04%.
The diff coverage is 84.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #614      +/-   ##
==========================================
+ Coverage   61.76%   61.81%   +0.04%     
==========================================
  Files          31       31              
  Lines        1810     1820      +10     
==========================================
+ Hits         1118     1125       +7     
- Misses        619      621       +2     
- Partials       73       74       +1     
Impacted Files Coverage Δ
internal/trigger/messagebus/messaging.go 72.15% <84.21%> (-0.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fb6bb54...165dc69. Read the comment docs.

@AlexCuse AlexCuse changed the title feat(messagebus): Allow Subscribe to Multiple Topics feat(sdk): Multi-topic messagebus subscriptions Dec 23, 2020
@AlexCuse AlexCuse force-pushed the messagebus-multiple-subscribe branch 2 times, most recently from 32be6e9 to 165dc69 Compare December 30, 2020 14:40
@AlexCuse AlexCuse marked this pull request as ready for review December 30, 2020 14:41
@AlexCuse AlexCuse force-pushed the messagebus-multiple-subscribe branch 2 times, most recently from 545fe44 to 92b338f Compare January 4, 2021 20:43
Change BindingInfo.SubscribeTopic to .SubscribeTopics, treat as comma
separated string.  Support multi-topic subscriptions in MQTT trigger
simply by looping on connect/reconnect, for messagebus trigger introduce
a "collector" channel.

Signed-off-by: Alex Ullrich <alex.ullrich@gmail.com>
@AlexCuse AlexCuse force-pushed the messagebus-multiple-subscribe branch from 92b338f to d780d8e Compare January 5, 2021 04:05
@lenny-goodell
Copy link
Member

@AlexCuse , Sorry I didn't see this PR before I did my own implementation.
Please see #625.

Let me know what you think of my vs yours.

@AlexCuse
Copy link
Contributor Author

AlexCuse commented Jan 6, 2021

LGTM @lenny-intel. The only thing I see that might be missing is testing the multi-topic aspect of the subscribe (apologies if I missed this just doing a quick scan on github here). Would be pretty easy to crib the test from my PR, or add it to another test case.

@rsdmike
Copy link
Member

rsdmike commented Jan 21, 2021

Closing due to #625

@rsdmike rsdmike closed this Jan 21, 2021
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.

4 participants