-
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
Conversation
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Signed-off-by: He Jie Xu <hejie.xu@intel.com>
/retest |
Retrying Azure Pipelines: |
/** | ||
* @return Connection ID of the downstream connection, or unset if not available. | ||
**/ | ||
virtual absl::optional<uint64_t> connectionID() const PURE; |
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 for downstreamAddressProvider().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.
return stream_info.downstreamAddressProvider().localAddress(); |
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.
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.
Thanks this is a great. A small question/comment.
/wait-any
/** | ||
* @return Connection ID of the downstream connection, or unset if not available. | ||
**/ | ||
virtual absl::optional<uint64_t> connectionID() const PURE; |
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?
@@ -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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
got it, updated, thanks for the review!
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.
Thanks!
…roxy#17231) Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Commit Message: socket: move connection id interface to SocketAddressProvider
Additional Description:
Avoid populating the connection id into streamInfo by multiple places, moving connection
id into SocketAddressProvider.
Risk Level: low
Testing: unittest
Docs Changes: n/a
Release Notes: n/a
Part of #17168
Signed-off-by: He Jie Xu hejie.xu@intel.com