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.
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.
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.
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.
I agree with @soulxu. I'm not sure there is any reason to have a facade here?
There was a problem hiding this comment.
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.
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.
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.
| /** | ||
| * @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.
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.
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.
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.
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.
got it, updated, thanks for the review!
…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