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

Add no subscribers fallback option to GoChannel Pub/Sub #418

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

annismckenzie
Copy link

@annismckenzie annismckenzie commented Jan 7, 2024

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 ☑️).

@@ -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 = "*"
Copy link
Member

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?

Copy link
Author

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.


// When true, messages sent to a topic without any subscribers will be sent to the
// subscribers of the `*` topic.
EnableFallback bool
Copy link
Member

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

Copy link
Author

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.


// When true, messages sent to a topic without any subscribers will be sent to the
// subscribers of the `*` topic.
EnableFallback bool
Copy link
Member

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?

Copy link
Author

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.

Copy link
Author

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.

@annismckenzie
Copy link
Author

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.
@annismckenzie annismckenzie force-pushed the add-no-subscribers-fallback-to-gochannel-pubsub branch from 7fa1b92 to c2ff17b Compare April 10, 2024 20:35
@annismckenzie
Copy link
Author

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.

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.

2 participants