-
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
matchers: implement interval tree for sublinear port range matching #19912
Conversation
Signed-off-by: Kuat Yessenov <kuat@google.com>
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to |
Signed-off-by: Kuat Yessenov <kuat@google.com>
Signed-off-by: Kuat Yessenov <kuat@google.com>
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 deps
Signed-off-by: Kuat Yessenov <kuat@google.com>
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.
Very cool stuff. A high level request about code comments before I review in depth. Also, please check the coverage report for this new code, I think you should be able to hit 100% coverage for all error branches. Thank you!
/wait
namespace Network { | ||
namespace IntervalTree { | ||
|
||
template <class Data, class N> class IntervalTree { |
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's very difficult/impossible to read this kind of code and understand what is going on as a new reader. Can you please add a large block comment at the top that describes what this does, provides a high level implementation overview, etc.? Also in general I would over comment the code below if possible.
Also in general this kind of code scares me a bit so you might consider writing a small fuzz test for it to help look for corner cases.
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
Commit Message: Data structure for efficient lookup of intervals containing a port in the unified matcher framework.
The data structure is a balanced tree of pivots so that:
To ensure balance, the pivots are chosen as medians in the sorted list of all start and end points.
Risk Level: low
Testing: unit, integration
Docs Changes: none (need #20277 )
Release Notes: none