grpc: implement TTL manager to re-buffer pending messages for bidirectional gRPC#18987
Conversation
…ages Signed-off-by: Shikugawa <rei@tetrate.io>
Shikugawa
changed the title
|
@yanavlasov PTAL |
yanavlasov
left a comment
yanavlasov
left a comment
There was a problem hiding this comment.
Is this PR complete? I do not see how the BufferedMessageTtlManager is being used. As such it is hard to reason about some of the decisions, like using flat_has_set of message ids in the TTL manager.
/wait-any
Yes. This is a part of supporting #17486. This feature will be utilized there. |
|
@yanavlasov Could you take a look? |
|
ping @yanavlasov |
|
/assign-from @envoyproxy/first-pass-reviewers |
|
@envoyproxy/first-pass-reviewers assignee is @wrowe |
|
@wrowe ping! |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks for working on this! The logic seems right to me, just a few comments to make this easier for people to understand
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks for iterating on this! A few more comments
| NiceMock<Upstream::MockClusterManager> cm_; | ||
| NiceMock<Http::MockAsyncClient> http_client_; | ||
| Http::MockAsyncClientStream http_stream_; | ||
| DangerousDeprecatedTestTime test_time_; |
There was a problem hiding this comment.
Can we use simulated test time here instead?
There was a problem hiding this comment.
Simulated test time is no longer needed here. Thus I deleted it.
| dispatcher_->run(Event::Dispatcher::RunType::Block); | ||
| EXPECT_EQ(ttl_manager_->deadlineForTest().size(), 0); | ||
|
|
||
| // Test if deadline queue is empty after queue cleared once. |
There was a problem hiding this comment.
Should this comment be before L54?
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks! A few comments on the documentation wording
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
A suggestion for the comment wording, otherwise LG
/wait
| // The TTL Manager can be given a set of IDs that are expected to expire at the same time. | ||
| // When checking for ID expiration, an expiration callback will be called for each ID belonging to | ||
| // this set of IDs. This class is used to manage the lifetime of messages stored in | ||
| // BufferedAsyncClient. Messages whose survival period has expired will be deleted from the Buffer. | ||
| // It allows monitoring a set of IDs for expiration, triggering a callback upon expiration. | ||
| // This class is able to monitor multiple sets of IDs at the same time, even after some of them have | ||
| // expired. When the callback is invoked, the callback given to the constructor will be will be | ||
| // executed for each of the TTL-elapsed IDs. After that, if the IDs to be monitored is not empty, | ||
| // the manager will continue to monitor for expiration again. |
There was a problem hiding this comment.
This still seems a bit repetitive, how about:
This class is used to manage the TTL for pending uploads within a BufferedAsyncClient. Multiple IDs
can be inserted into the TTL manager at once, all with the same TTL (specified in the constructor). Upon expiry,
the TTL manager will invoke the provided expiry callback for each ID. Note that there is no way to disable
the expiration, and so it's up to the recipient of the callback to handle this. BufferedAsyncClient will do
the right thing here: if the expired ID is still in flight it will be returned to the buffer, otherwise it does nothing.
The TTL manager is designed to handle multiple sets of IDs inserted at various times, backing this with a
single Timer. This allows us to track a large amount of IDs inserted at different times without using a lot of
different timers, which could put undue pressure on the event loop.
?
There was a problem hiding this comment.
Thank you for your suggestion!
…tional gRPC (envoyproxy#18987) Signed-off-by: Shikugawa <rei@tetrate.io> Signed-off-by: Josh Perry <josh.perry@mx.com>
8 participants
Signed-off-by: Shikugawa rei@tetrate.io
Commit Message: grpc: implement TTL manager to re-buffer pending messages for bidirectional gRPC
Additional Description: Related with #17486. This PR provides the TTL manager for buffered bidirectional gRPC messages.
Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]