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

De-couple Msg from private Subscriber implementation #196

Closed
wants to merge 1 commit into from

Conversation

23caterpie
Copy link

Based on the comments made on this Pull request: https://github.com/nats-io/go-nats-streaming/pull/195#issuecomment-411948113

This allows making use of the Subscription interface for message acknowledgment.

Currently, Msg's Ack() method assumes that its Subcriber is typed
as the internal subscription structure and will panic is such is
not the case. This makes using a mock subscriber unsafe and
impossible without type checking code forks.

Since the code within Msg's Ack() is intrinsically tied to the
subscriber struct, it made sense to convert that code into a
method of subscriber. Msg will now be acknowledged based on the
definition of its Subscriber.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.863% when pulling b154f68 on 23caterpie:master into e6ff826 on nats-io:master.

@kozlovic
Copy link
Member

I was mentioning the OP to use that in this code. If it was part of the API it would be Ack() to be exported. Any reason you use *pb.MsgProto instead of *Msg?

(I like the idea to have the Ack() part of the subscription and not the message. Actually, in the C NATS Streaming Client, this is what I did, not a function of the message but the subscription).

@23caterpie
Copy link
Author

I made ack() an un-exported method to keep the current flow of using the *Msg's Ack() for the acknowledgement for simplicity instead of providing 2 different ways to do it. This also ensures that the message is acknowledged using it's own Subscriber.

With it being un-exported, it felt alright to use the proto structure since that's all we need to make the *Ack object, and passing an object that has a Subscription to a Subscription method feels like things could get messy.

If you would rather make it a public method, then taking a *Msg would make more sense.

@kozlovic
Copy link
Member

Actually, it's true that there would be a risk of acknowledging a message that was not receive by this subscription, which is bad.

Again, at this point I don't think I will merge this PR. I am not too happy with the current API (namely, we should not have passed a NATS connection, but options, we have added NATS Subscriptions APIs to surface the internals because a user needed that, etc), and adding this does help in making it cleaner.

As described in other PR, you can use an helper if you want to use a mocked subscription.

@kozlovic kozlovic closed this Mar 19, 2019
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