stream_info: cleaning up setters and getters#19139
stream_info: cleaning up setters and getters#19139yanavlasov merged 6 commits intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
force-pushed
the
upstream3
branch
from
07a7c62 to
3022553
Compare
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
force-pushed
the
upstream3
branch
from
3022553 to
abae1f6
Compare
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
requested review from
RyanTheOptimist,
asraa,
davinci26,
ggreenway,
mathetake and
mattklein123
as code owners
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
There's a lot going on here, but it looks good. I think it needs a main merge though?
| * information for the one that "wins". | ||
| */ | ||
| virtual void setUpstreamTiming(const UpstreamTiming& upstream_timing) PURE; | ||
|
|
There was a problem hiding this comment.
This PR seems quite similar to #19118? Does this PR replace that one?
There was a problem hiding this comment.
yeah, theoretically. there's 2 issues with that one - the clang tidy which I have fixed locally (not pushed) and the windows build failure, which I could not figure out. I was hoping removing the getters would remove the build failure but left it assigned my way until I was sure.
if it's too big to combine getter and setter clean up, I can likely submit removing getters first, but didn't want to break it up until 1) I knew it fixed the problem and 2) checking to see if you wanted it split or were OK with the one large PR
There was a problem hiding this comment.
This approach seems totally fine. No need to split. It's a lot of movement but it's largely mechanical/straightforward. It's not like there's lots of complex interactions which are changing, I think.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
/retest |
|
Retrying Azure Pipelines: |
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
LGTM, Though I think there's a missing EXPECT_CALL in //test/extensions/filters/http/alternate_protocols_cache:filter_test
| * information for the one that "wins". | ||
| */ | ||
| virtual void setUpstreamTiming(const UpstreamTiming& upstream_timing) PURE; | ||
|
|
There was a problem hiding this comment.
This approach seems totally fine. No need to split. It's a lot of movement but it's largely mechanical/straightforward. It's not like there's lots of complex interactions which are changing, I think.
| [](const StreamInfo::StreamInfo& stream_info) | ||
| -> std::shared_ptr<const Envoy::Network::Address::Instance> { | ||
| if (stream_info.upstreamInfo().has_value()) { | ||
| return stream_info.upstreamInfo().value().get().upstreamLocalAddress(); |
There was a problem hiding this comment.
Nit: small simplification, since OptRef already has a method for this chain of calls.
| return stream_info.upstreamInfo().value().get().upstreamLocalAddress(); | |
| return stream_info.upstreamInfo().ref().upstreamLocalAddress(); |
- Update .bazelrc - envoyproxy/envoy#19185 - abstracts address_ from DnsResponse into a sub-struct AddrInfoResponse - envoyproxy/envoy#19139 - removes setUpstreamTiming and instead we set an UpstreamInfo shared pointer on the StreamInfo and modify that one directly - Sync tools/gen_compilation_database.py and add instructions to MAINTAINERS.md making sure we always keep that in sync Signed-off-by: Nathan Perry <nbperry@google.com>
3 participants
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]