-
Notifications
You must be signed in to change notification settings - Fork 402
feat(PubSub): Add NackImmediately and WaitForProcessing shutdown mode #15482
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,24 @@ public enum Reply | |
| Ack = 1, | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Settings available for subscriber shutdown. | ||
| /// </summary> | ||
| public enum SubscriberShutdownSetting | ||
| { | ||
| /// <summary> | ||
| /// Stops streaming new upstream messages and then continues processing all received messages. If there are | ||
| /// still messages that need to be processed 30s before the timeout is reached it will switch to <see cref="NackImmediately"/>. | ||
| /// </summary> | ||
|
Comment on lines
+63
to
+65
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, there's no timeout here, so referencing the timeout is unclear. The explanation re switching to nack inmediately is likely better suited for the method itself. |
||
| WaitForProcessing = 0, | ||
|
|
||
| /// <summary> | ||
| /// Stops streaming new upstream messages and then aggressively releases unhandled messages by sending Nack responses. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment re: upstream. And avoid using aggressively. Just say that it nacks all unhandled messages. |
||
| /// Already handled messages will still be acknowledged. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The client app response to a handle message may have been nack, so here something like "the ack/nack will be sent". And are we certain about this? |
||
| /// </summary> | ||
| NackImmediately = 1, | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Default <see cref="FlowControlSettings"/> for <see cref="SubscriberClient"/>. | ||
| /// Allows 1,000 outstanding messages; and 100Mb outstanding bytes. | ||
|
|
@@ -217,6 +235,7 @@ public virtual Task StartAsync(Func<PubsubMessage, CancellationToken, Task<Reply | |
| /// <param name="hardStopToken">Cancel this <see cref="CancellationToken"/> to abort handlers and acknowledgement.</param> | ||
| /// <returns>A <see cref="Task"/> that completes when all handled messages have been acknowledged; | ||
| /// faults on unrecoverable service errors; or cancels if <paramref name="hardStopToken"/> is cancelled.</returns> | ||
| [Obsolete("Use StopAsync(SubscriberShutdownSetting, TimeSpan?, CancellationToken) instead.")] | ||
| public virtual Task StopAsync(CancellationToken hardStopToken) => throw new NotImplementedException(); | ||
|
|
||
| /// <summary> | ||
|
|
@@ -229,7 +248,19 @@ public virtual Task StartAsync(Func<PubsubMessage, CancellationToken, Task<Reply | |
| /// <param name="timeout">After this period, abort handling and acknowledging messages.</param> | ||
| /// <returns>A <see cref="Task"/> that completes when all handled messages have been acknowledged; | ||
| /// faults on unrecoverable service errors; or cancels if <paramref name="timeout"/> expires.</returns> | ||
| public virtual Task StopAsync(TimeSpan timeout) => StopAsync(new CancellationTokenSource(timeout).Token); | ||
| [Obsolete("Use StopAsync(SubscriberShutdownSetting, TimeSpan?, CancellationToken) instead.")] | ||
| public virtual Task StopAsync(TimeSpan timeout) => StopAsync(SubscriberShutdownSetting.WaitForProcessing, timeout); | ||
|
|
||
| /// <summary> | ||
| /// Stop this <see cref="SubscriberClient"/>. | ||
| /// The returned <see cref="Task"/> completes when all handled messages have been acknowledged. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, or nacked. But also, this is not true, it depends on what the settings are. |
||
| /// The returned <see cref="Task"/> faults if there is an unrecoverable error with the underlying service. | ||
| /// </summary> | ||
| /// <param name="shutdownSetting">The <see cref="SubscriberShutdownSetting"/> to use for shutdown.</param> | ||
| /// <param name="timeout">Optional. The timeout for the shutdown process. If not specified, a default 1-hour timeout is used.</param> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is this 1 hour coming from? Also, more than an optional parameter, I think I'd prefer that it accepted null. |
||
| /// <param name="cancellationToken">Optional. A <see cref="CancellationToken"/> that can be used to abort the graceful shutdown and trigger an immediate hard stop.</param> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not so sure about this but let's chat. |
||
| /// <returns>A <see cref="Task"/> that completes when the subscriber is stopped, or if an unrecoverable error occurs.</returns> | ||
| public virtual Task StopAsync(SubscriberShutdownSetting shutdownSetting, TimeSpan? timeout = null, CancellationToken cancellationToken = default) => throw new NotImplementedException(); | ||
|
|
||
| /// <summary> | ||
| /// Disposes this <see cref="SubscriberClient"/> asynchronously. | ||
|
|
||
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.
Consider
ShutdownOption, mostly dropping subscription given that this is a type defined on theSubscriptionClient.