-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
udp_proxy: implement idle timeout and some stats #8999
Conversation
Another bunch of work towards #492. The remaining work is proper wiring up of upstream cluster management, host health, etc. and documentation. This will be done in the next PR. Signed-off-by: Matt Klein <[email protected]>
@danzh2010 @goaway PTAL |
Signed-off-by: Matt Klein <[email protected]>
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.
@envoyproxy/api-shepherds API LGTM modulo nits. There are breaking changes, but this is a v2alpha
API.
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
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.
Sorry for the delay. Looks good over all with a few nits.
Signed-off-by: Matt Klein <[email protected]>
Signed-off-by: Matt Klein <[email protected]>
@danzh2010 updated PTAL |
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.
LGTM
@@ -76,6 +111,12 @@ class UdpProxyFilter : public Network::UdpListenerReadFilter, Logger::Loggable<L | |||
UdpProxyFilter& parent_; | |||
const Network::UdpRecvData::LocalPeerAddresses addresses_; | |||
const Upstream::HostConstSharedPtr host_; | |||
// TODO(mattklein123): Consider replacing an idle timer for each session with a last used | |||
// time stamp and a periodic scan of all sessions to look for timeouts. This solution is simple, |
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.
Might consider to use linked hash map to hold all the sessions in that case.
It can also be one alarm per session, but scheduled once per loop instead of once per packet. Not necessarily to be a common idle timer cross all sessions.
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.
Agreed there is a bunch we can do. Let's tackle this in a follow up. Thanks for the good discussion. :)
Signed-off-by: Matt Klein <[email protected]>
Another bunch of work towards
#492.
The remaining work is proper wiring up of upstream cluster
management, host health, etc. and documentation. This will
be done in the next PR.
Risk Level: Low
Testing: New integration/unit tests
Docs Changes: N/A
Release Notes: N/A