-
Notifications
You must be signed in to change notification settings - Fork 688
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
Added BackOff helper method to set backoff through subOpts #933
Conversation
mfaizanse
commented
Mar 22, 2022
- Added BackOff helper method to set backoff through subOpts
What is the goal of this change? For true push I am worried that this could collide with flow (or lack thereof) and cause memory bloat in the client app. Could be useful maybe for when you do a pull request however but best to understand what the exact intent is here. |
The NAK delay and backOff is implemented in these PRs: nats-server and nats-go. Currently, in the API for to create a consumer, we can set the
|
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.
Would add a test and either remove from example or "fix" it as described in the comments.
@derekcollison We (I think I did) added the features (Nak and Backoff) but I simply forgot to add the option. So I think @mfaizanse is just fixing my omission here :-)
example_test.go
Outdated
// Start delivering messages with delay based on BackOff array of time durations. | ||
js.Subscribe("foo", func(msg *nats.Msg) { | ||
fmt.Printf("Received a message: %s\n", string(msg.Data)) | ||
}, nats.BackOff([]time.Duration{50 * time.Millisecond, 250 * time.Millisecond})) |
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.
This complies, but would actually not "work" because you would need to set the MaxDeliver to at least 2 (or more), but also add nats.ManualAck()
because otherwise the callback would auto-ack anyway, so it is unlikely that there would be redeliveries...
So instead (or at least in addition to) having this here, I would update, say TestJetStreamSubscribe_AckPolicy test to use the nats.BackOff() new option. In this test (at the end of it), we use backoff but with the AddConsumer directly.
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.
LGTM
@derekcollison I have approved this PR, let me know if you still have concerns: we have added the BackOff list consumer configuration in the client, so not sure why is that an issue to make it an option for the js.Subscribe() calls... |