-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
socket: move connection id interface to SocketAddressProvider #17231
Changes from 5 commits
8666914
957631d
0baa14c
e1a583b
ac01cc5
f6d7982
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,8 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt | |
Event::FileReadyType::Read | Event::FileReadyType::Write); | ||
|
||
transport_socket_->setTransportSocketCallbacks(*this); | ||
|
||
socket_->addressProvider().setConnectionID(id()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be a much larger change, but is there any reason to not just have the ID be generated as part of the "address provider" and then we don't need a setter or any of the optional stuff? Perhaps a TODO if you agree with that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So we need to move the 'next_global_id_' (at line 69) into the SocketAddressProvider. And SocketImpl will create an instance of SocketAddressProvider, which means every socket will have a connection id. The only thing I found is ListenerSocket is based on SocketImpl, so every ListenerSocket will have a connection id, which doesn't match the concept 'connection' a little bit. If we feel this is ok, then this should be doable. WDYT, if you think it is ok, let me add a TODO, then I will work on it on a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah IMO this would be better overall. I would just add a small TODO and we can potentially do this later. It's not a huge deal. Thank you! /wait There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, updated, thanks for the review! |
||
} | ||
|
||
ConnectionImpl::~ConnectionImpl() { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not keep
connectionID()
around, but make it a facade fordownstreamAddressProvider().connectionID()
call? This way StreamInfo gets uncoupled from an AddressProvider (see here for an example of such a coupling: https://github.com/envoyproxy/envoy/pull/17231/files#diff-888dc1d9b82ce89abbe565c8c52a11dd16120d2d23b794e1a8c46e5ac0bb039aR817)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
emm...I'm hesitant when writing the PR. I don't a strong reason to keep the StreamInfo::connectionID or not. In the end, I follow the localAddress()/remoteAddress(), they don't have a facade in StreamInfo.
envoy/source/common/formatter/substitution_formatter.cc
Line 782 in a0ca08b
If we keep connectionID, I thought it should be able uncouple the calling code and AddressProvider. But AddressProvider interface is quite single, so I think it should be ok. But let me know how you see the benefit of uncouple streaminfo and addressProvider, I will update the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @soulxu. I'm not sure there is any reason to have a facade here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR alone the chain of calls
stream_info.downstreamAddressProvider()...
is used in five places, not counting tests. There are probably going to be more with time? It's not huge for sure, but makes changing and testing the code harder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with both ways, if we have a facade for connection id, what about those localaddress/remoteaddress? @mattklein123 what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it doesn't really matter, but all things being equal I don't think it's better to have more methods vs. some slightly longer call chains. I think it's fine as is.