async client: refactor Http::AsyncClient::Callbacks to pass in Http::AsyncClient::Request* for correlation between requests and responses#10358
Conversation
yskopets
requested review from
alyssawilk,
dio,
lizan and
mattklein123
as code owners
yskopets
force-pushed
the
refactoring/async-client-callbacks
branch
from
43e4394 to
4dfe8fd
Compare
…p::AsyncClient::Request*` for correlation between requests and responses Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
…s` and `AsyncClient::RequestCallbacks` Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
yskopets
force-pushed
the
refactoring/async-client-callbacks
branch
from
c00cd58 to
60d8e85
Compare
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Thanks, this seems like the right direction. Couple comments
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
yskopets
force-pushed
the
refactoring/async-client-callbacks
branch
from
b100a3e to
33db494
Compare
alyssawilk
left a comment
alyssawilk
left a comment
There was a problem hiding this comment.
One drive by question - do we have any tests of multiple requests + responses where we can regression test they're being correlated correctly?
|
@alyssawilk No, sorry, I don't have such a test. I'll see what I can do. |
…d responses Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
|
@alyssawilk I've added missing unit tests. |
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
This looks good to me besides the one small comment.
@alyssawilk Does the testing story seem okay here or would you like to see an integration test?
| const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* { | ||
| callbacks.onFailure(Envoy::Http::AsyncClient::FailureReason::Reset); | ||
| callbacks.onFailure(request_, Envoy::Http::AsyncClient::FailureReason::Reset); | ||
| return nullptr; |
There was a problem hiding this comment.
should you be returning the request_ ptr here?
There was a problem hiding this comment.
Actually, I've decided to restore original return nullptr in all other places.
return nullptr affects the code path that follows AsyncClient::send(), so I shouldn't have changed it in the first place.
Signed-off-by: Yaroslav Skopets <yaroslav@tetrate.io>
3 participants
Description: refactor
Http::AsyncClient::Callbacksto pass inHttp::AsyncClient::Request*for correlation between requests and responsesRisk Level: Low
Testing: unit tests
Docs Changes: N/A
Release Notes: N/A
Context:
Motivation:
Callbacksimplementations to be able to determine which of the multiple in-flight requests has changed it's status