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

Implement FanIn #221

Merged
merged 9 commits into from Jan 26, 2022
Merged

Implement FanIn #221

merged 9 commits into from Jan 26, 2022

Conversation

0michalsokolowski0
Copy link
Contributor

@0michalsokolowski0 0michalsokolowski0 commented Feb 28, 2021

Create component multiplexing messages from many topics on one

@0michalsokolowski0 0michalsokolowski0 marked this pull request as ready for review March 14, 2021 11:13
}

// Subscribe implements the standard Subscriber interface.
func (f *FanIn) Subscribe(ctx context.Context, topic string) (<-chan *message.Message, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this method still make sense for FanIn? It seems unused now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also came to my mind, how it should be used 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah oh, I got it.

Maybe it would be better to not expose subscriber here, but just provide higher level API that is doing that under the hood? In other words - to be able to use FanIn without creating the router. So in that case I could just do

fanin, err := gochannel.NewFanIn(upstreamPubSub, config, logger)
// ...
fanin.Run()

In that case API may be much simpler to use. But the question is if in that case it fits to your use case ;-)

@@ -0,0 +1,139 @@
package gochannel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it's not specific for a gochannel, I think that it may be worth to move to https://github.com/ThreeDotsLabs/watermill/tree/master/components 😉

// AddSubscription adds an internal subscriptions.
// You need to call this method with slice of `fromTopics` and `toTopic` before the FanIn is started.
// AddSubscription is idempotent.
func (f *FanIn) AddSubscription(fromTopics []string, toTopic string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to call this method with slice of fromTopics and toTopic before the FanIn is started.

so maybe it's better to add it to the constructor (and add some kind of config). In that case design would be simpler 😉

@roblaszczak roblaszczak marked this pull request as draft October 9, 2021 13:58
@roblaszczak
Copy link
Member

As it's still in progress I converted it to draft.

@0michalsokolowski0 0michalsokolowski0 marked this pull request as ready for review November 28, 2021 19:48
@roblaszczak
Copy link
Member

Hey @0michalsokolowski0, sorry for a bit of delay with the review 😅

All good, thanks for the contribution 🍻

@roblaszczak roblaszczak merged commit 54d4b23 into ThreeDotsLabs:master Jan 26, 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

3 participants