Skip to content
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

Merged
merged 12 commits into from
Nov 25, 2019
Merged

Conversation

mattklein123
Copy link
Member

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

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]>
@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8999 was opened by mattklein123.

see: more, trace.

@mattklein123
Copy link
Member Author

@danzh2010 @goaway PTAL

Copy link
Member

@htuch htuch left a 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.

Copy link
Contributor

@danzh2010 danzh2010 left a 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.

@mattklein123
Copy link
Member Author

@danzh2010 updated PTAL

Copy link
Contributor

@danzh2010 danzh2010 left a 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,
Copy link
Contributor

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.

Copy link
Member Author

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. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants