-
Notifications
You must be signed in to change notification settings - Fork 406
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
Add no subscribers fallback option to GoChannel Pub/Sub #418
base: master
Are you sure you want to change the base?
Add no subscribers fallback option to GoChannel Pub/Sub #418
Conversation
pubsub/gochannel/pubsub.go
Outdated
@@ -11,6 +11,10 @@ import ( | |||
"github.com/ThreeDotsLabs/watermill/message" | |||
) | |||
|
|||
// NoSubscribersFallbackTopic is the fallback topic messages without any subscribers will be sent to. | |||
// This is used if the `EnableFallback` configuration option is enabled. | |||
const NoSubscribersFallbackTopic = "*" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think about adding configuration for the topic name instead? so we could have two options in Config
EnableFallback
NoSubscribersFallbackTopic
what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added both EnableNoSubscribersFallback
(as per your review comment below) and NoSubscribersFallbackTopic
to the configuration. I did want to leave the default topic (*
) as a const in case the user doesn't set a custom fallback topic. I also added another test case for this.
pubsub/gochannel/pubsub.go
Outdated
|
||
// When true, messages sent to a topic without any subscribers will be sent to the | ||
// subscribers of the `*` topic. | ||
EnableFallback bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be a bit longer, IMO it could be a bit more explicit, like EnableNoSubscribersFallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I agree that the longer version is more expressive and changed it.
pubsub/gochannel/pubsub.go
Outdated
|
||
// When true, messages sent to a topic without any subscribers will be sent to the | ||
// subscribers of the `*` topic. | ||
EnableFallback bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, food for thought, but I wonder if this fallback couldn't be a closure 🤔
like NoSubscribersFallback func(msg message.Message, pubsub *GoChannel) error
It could be much more flexible 🤔 WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds worthwhile to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I investigated this with a private project of mine where I'm using the fork and the closure is awkward to use because the function you want to call has to be known and available when instantiating the GoChannel
. However, I'm passing this GoChannel
object around for subscribing and publishing and then just use
noSubscribersMessages, err := pubSub.Subscribe(ctx, gochannel.NoSubscribersFallbackDefaultTopic)
if err != nil {
return fmt.Errorf("subscribing to fallback topic: %w", err)
}
go func(ctx context.Context, messages <-chan *message.Message) {
for {
select {
case msg := <-messages:
// handle message
// lastly:
msg.Ack()
case <-ctx.Done():
return
}
}
}(ctx, noSubscribersMessages)
which works well and is easy to use. That is to say, I'd leave this as is.
Just saw your review – thank you! I’ll take a closer look next week. |
If the option is enabled, messages sent to topic without any subscribers will be forwarded to the `*` topic subscribers.
This goes back to the simple if and an early return when the option isn't enabled.
7fa1b92
to
c2ff17b
Compare
Short update: I rebased and addressed your comments, see above. I also updated a couple comments as promised in the pull request description in 8808fd7 and this would be ready for another look. |
I needed a way to listen to any message sent via the GoChannel Pub/Sub that didn't have a subscriber yet. This was mainly used for debugging but it could be useful for other purposes as well.
I made sure to add a test and left the rest of the tests untouched – the test suite is passing.
Would you be open to accepting this contribution? If so, I'd take another pass over the current implementation's comments, some would need updating (for example,
That means if you send a message to a topic to which no subscriber is subscribed, that message will be discarded.
– update: this is done ☑️).