Pubsub: Update default settings; add maximum total lease extension #3920
Conversation
googlebot
added
the
cla: yes
| @@ -207,10 +218,15 @@ internal void Validate() | |||
| public static TimeSpan MinimumAckExtensionWindow { get; } = TimeSpan.FromMilliseconds(50); | |||
|
|
|||
| /// <summary> | |||
| /// The default message ACKnowlegdment extension window of 15 seconds. | |||
| /// The default message ACKnowlegdement extension window of 15 seconds. | |||
There was a problem hiding this comment.
There's still a typo here - gd instead of dg
There was a problem hiding this comment.
Oops, thanks. Done!
| } | ||
| } | ||
| })); |
There was a problem hiding this comment.
Should the method return at this point? Presumably it's just about feasible for _maxExtensionDuration to be one tick, so it could be disposed by the end of the block.
There was a problem hiding this comment.
No, the code here is setting up the max lease-extension deadline, so at this point it is correct to continue with sending the extension request to the server, and scheduling a further extension.
Comments added to clarify.
| { | ||
| cts2 = _cts; | ||
| } | ||
| cts2?.Cancel(); |
There was a problem hiding this comment.
Should this dispose as well? I'm a little lost as to what this is all doing.
There was a problem hiding this comment.
(Comments on this and Dispose would be useful.)
There was a problem hiding this comment.
Reading through everything else, it looks like it doesn't have to be disposed, but I suspect it would be good to do so.
There was a problem hiding this comment.
Various comments added to try to clarify how this works.
| { | ||
| // Ids have been added to _extendQueue, so trigger a push. | ||
| _eventPush.Set(); | ||
| // Some ids still exist, schedule another extension. |
There was a problem hiding this comment.
Possibly edit this comment to add "with the same deadline for how long the lease can be extended"?
No description provided.