router: set upstream host from the conn pool #33790
Conversation
|
/wait |
|
|
|
multiple addresses.... |
Signed-off-by: wbpcode <wbphub@live.com>
wbpcode
force-pushed
the
dev-upstream-request-set-host
branch
from
9724057
to
c0a47d0
Compare
Signed-off-by: wbpcode <wbphub@live.com>
|
/wait fix ci |
Signed-off-by: wbpcode <wbphub@live.com>
|
/wait ci |
Signed-off-by: wbpcode <wbphub@live.com>
| @@ -195,6 +195,7 @@ class UpstreamRequest : public Logger::Loggable<Logger::Id::router>, | |||
| std::unique_ptr<GenericUpstream> upstream_; | |||
| absl::optional<Http::StreamResetReason> deferred_reset_reason_; | |||
| Upstream::HostDescriptionConstSharedPtr upstream_host_; | |||
| std::shared_ptr<StreamInfo::UpstreamInfoImpl> upstream_info_; | |||
There was a problem hiding this comment.
if we're going to eagerly create this, I think we can nix the local upsteam_host_ to not have it stored in many places.
| @@ -128,9 +129,9 @@ UpstreamRequest::UpstreamRequest(RouterFilterInterface& parent, | |||
| parent_.callbacks()->activeSpan().injectContext(trace_context, upstream_context); | |||
| } | |||
|
|
|||
| stream_info_.setUpstreamInfo(std::make_shared<StreamInfo::UpstreamInfoImpl>()); | |||
| stream_info_.setUpstreamInfo(upstream_info_); | |||
There was a problem hiding this comment.
why create this early but not set it early? Worth code comments I think
There was a problem hiding this comment.
This PR actually just set the upstream host early. The upstream info is still be created at the constructor (we just moved it more the constructor function to the constructor initialize list)?
| // the host description from the connection pool callback. Exception is the logical host | ||
| // which's addresses may be changed at runtime and the host description from the connection | ||
| // pool callback will be different from the host description from host() of connection pool. | ||
| if (real_host != nullptr && real_host != upstream_host_) { |
There was a problem hiding this comment.
if we're latching host early but it may change, can this be a problem for folks latched to the old version? Can you comment on why you want to latch early?
There was a problem hiding this comment.
Can you comment on why you want to latch early?
It is because the problem in our actually production scenario.
Considering a scenario like this, if the request is timeout before any pool callbacks are called, then the upstream host will never be set and we can even cannot know which host is selected for this request from the access log.
if we're latching host early but it may change, can this be a problem for folks latched to the old version?
The real host may have different address with the early one. All other fields or content are same. If some one get the host after the upstream request is created and before the pool callbacks is called, the host may have different addresses with the final one.
The upstream host in the upstream info mainly is used for logging, tracing, etc. So, I think the early one at least is better than null.
But I am open to this issue. It's would be great for me if we can figure out a better way to resolve above problem.
There was a problem hiding this comment.
Hah, may be can check if the upstream info has a valid upstream host when the upstream request is reset (remote or local), and if the upstream host is nullptr, we can get one from connection pool and set it to upstream info. WDYT?
There was a problem hiding this comment.
or is there a way of not snagging logical host? or subscribing to updates so it's always kept up to date, or actually grabbing the outer abstraction instead of a handle which can change? I think I'm OK with it changing if it must, but if we go that route we should probably also comment where the member is defined.
There was a problem hiding this comment.
For the logical host, when the connection is ready, the host will create a RealHostDescrition that wrap the logical host and the actual address that used by the connection.
This is a way to ensure the finally HostDescrition will always has an accurate address when logic host is used. Other types host needn't this because we assume it have a only one address.
However, with the support of the multiple-addresses of host, the correct way to get the accurate address of upstream host is the Network::ConnectionInfoProvider parameter of pool callbacks, for all types host.
So, I doubt the value of the RealHostDescrition now. Maybe we could remove the RealHostDesription directly and then the host from pool and the host from pool callbacks would be same. (And this would simplify the logic/complexity here, I taken some time to read the code, then found the difference between these hosts)
There was a problem hiding this comment.
I think this may need @mattklein123 - I'm not terribly familiar with the subtleties of realhostdescripton and local hosts
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
github-actions
bot
added
the
stale
|
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Commit Message: router: set upstream host from the conn pool
Additional Description:
The
host()method of connection pool will return a host in current implementation. We can set the host at constructor ofUpstreamRequestand needn't to wait the pool callbacks.Considering a scenario like this, if the request is timeout before any pool callbacks are called, then the upstream host will never be set and we can even cannot know which host is selected for this request from the access log.
Risk Level: mid, core logic change.
Testing: wait.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.