[ADDED] js_PullSubscribeAsync #785
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #785 +/- ##
==========================================
+ Coverage 68.71% 70.63% +1.91%
==========================================
Files 39 47 +8
Lines 15207 15394 +187
Branches 3143 3163 +20
==========================================
+ Hits 10449 10873 +424
+ Misses 1700 1465 -235
+ Partials 3058 3056 -2 ☔ View full report in Codecov by Sentry. |
|
|
|
@kozlovic I resolved the comments where I think I added obvious fixes for the feedback, please resolve the remaining as you find fit. I added tests for handling connect and reconnect while fetching, and made sure that OnFetchComplete handler is called if/when the sub is closed for whatever reason(s). There are some flappers registering on Travis in |
examples/js-sub.c
Outdated
| jsOpts.PullSubscribeAsync.KeepAhead = 7; | ||
| jsOpts.PullSubscribeAsync.CompleteHandler = _completeFetchCb; | ||
|
|
||
| jsFetchRequest lifetime; |
There was a problem hiding this comment.
@kozlovic @Jarema @mtmk I chose to have a separate lifetime parameter as a shortcut for passing these options. 2/5 I am considering removing the extra parameter, and adding all these values to jsOpts.PullSubscribeAsync. (a) It would be more idiomatic; (b) arguably, CompleteHandler a more common use-case and they should be "level"-ed; and (c) the use of jsFetchRequest struct may be confusing since no such request will ever be made.
What do you think?
There was a problem hiding this comment.
You could. The thing I like about the options, is if we decide down the line that we need other option(s), then the function definition stays the same. We don't have to come up with a different function name, etc..
That being said, you could pass the parameters that seem essential at this time, and if in the future we need more, then add them through the options?
There was a problem hiding this comment.
Replaced lifetime with more jsOpts, looks cleaner.
- Properly account for *received* fetch messages/bytes - Fetch status set to NATS_CONNECTION_CLOSED if fetch is terminated by a disconnect - Auto-Unsubscribe when the (pull async) subscription reaches the end of life(*) - Handle errors from sending fetch requests(*) - Disallow MaxBytes and KeepAhead simultaneously so we can set MaxBytes on subsequent requests accurately - Unrelated: _unsubscribe should not stop timers for max>0
|
@wallyqs FYI, many GHA jobs are still failing to download server images. |
There was a problem hiding this comment.
LGTM. By the way, we may want to add to the list of disabled Windows warnings in the CMakeLists.txt: add_compile_options(/wd4204) # non-constant aggregate initializer. This is for when we do something like: myStruct s = {"a", i}; (when one of the initialization value is not a constant)
| // Defalut values, change as needed. | ||
| jsOpts.PullSubscribeAsync.FetchSize = 128; // ask for 128 messages at a time | ||
| jsOpts.PullSubscribeAsync.NoWait = false; | ||
| jsOpts.PullSubscribeAsync.Timeout = 0; // for the entire subscription, in milliseconds |
There was a problem hiding this comment.
This is fine,
But I think that each pull request should have 30s expires default value.
Otherwise if server looses the Pull Requst, client will never know about it.
Similarly, HeartBeats should be enabled by default, with a value of 15 seconds (half of expires, which is maximum).
There was a problem hiding this comment.
@Jarema First, 1/5 I don't think that subscription-level Expires/Timeout should be defaulted on. By default, a sub should live until Unsubscribe, that's the idiom. A separate FetchTimeout (was Batch a better name?) would solve this issue but also add complexity and potential confusion.
Second, if the heartbeats are ON, and the server loses a pull request, the client will know about because of the missing heartbeat error. So, if we make heartbeat ON by default at some number of seconds why add a default timeout?
There is also a small issue of how to package "No Heartbeat" if the 0 value is defaulted on. A separate flag also adds complexity and may lead to confusion. Hearbeat=-1 would not be very intention-revealing IMO.
There was a problem hiding this comment.
I agree on not having timeout on subscription. In fact, none other clients have that option at all.
If you want to stop subscription, stop it explicitly, or do drain. We never do timeouts like thoge.
What I meant however, is expires value for each pull requst, and heartbeats for those pull requests too.
A replacement for #773 that seems to have lost its proper base in GitHub after changing it.