-
Notifications
You must be signed in to change notification settings - Fork 847
Add a NextHop parents yaml parser for use with remap. #4099
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
Conversation
2e99fa9 to
cb6db47
Compare
|
[approve ci autest] |
cb6db47 to
a228705
Compare
|
[approve ci autest] |
bc08898 to
0e7fc42
Compare
0e7fc42 to
156d054
Compare
|
[approve ci autest] |
|
+1 |
SolidWallOfCode
left a comment
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.
A key question is whether it's better to do so much of the parsing work in the converters or not. I did as little work as possible there and put most of the parsing logic elsewhere.
proxy/http/remap/NextHopConfig.cc
Outdated
|
|
||
| #include "ts/Diags.h" | ||
|
|
||
| #define BUFSIZE 8192 |
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.
Use constexpr, not #define. But better, don't use fixed sized buffers.
| decode(const Node &node, NextHopStrategyConfig &cfg) | ||
| { | ||
| YAML::Node strategy = node; | ||
| try { |
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.
This is a bad API design by YAMLCPP IMHO. It means either map access is fraught or you have to do it twice, e.g.
if (!node[NH_strategy]) { ...
} else { // error - missing key
}
I suspect, though, the thrown exception has the data you need and you don't have to catch and rethrow for every access.
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.
that was my mistake, twice is not necessary.
proxy/http/remap/NextHopConfig.cc
Outdated
| } | ||
| if (strategy.Type() == YAML::NodeType::Map) { | ||
| // verify keys | ||
| for (const_iterator it = strategy.begin(); it != strategy.end(); it++) { |
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.
You can use range for loops on the nodes -
for ( auto const& item : strategy ) {
if (std::none_of(... item.first.as<std::string>()) ) ...
| try { | ||
| strategy = node[NH_strategy]; | ||
| } catch (std::exception &ex) { | ||
| throw std::runtime_error("the required 'strategy' node does not exist in the yaml document."); |
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.
Look at the Mark method, which gives you access to the line and column location of the node, which is very nice for error messages.
|
That's a first pass of commentary. One additional note - I don't like the fixed buffer size approach to reading the configuration files. I think it's better to do something like |
proxy/http/remap/NextHopConfig.cc
Outdated
| // verify the keys | ||
| for (const_iterator it = node.begin(); it != node.end(); it++) { | ||
| if (std::none_of(valid_host_keys.begin(), valid_host_keys.end(), | ||
| [it](std::string s) { return s == it->first.as<std::string>(); })) { |
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.
const std::string &
| p1: &p1 | ||
| host: p1-cache.foo.com | ||
| protocol: | ||
| - http: 80 |
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.
Support there are two "http" ports, say 80 and 3128. How would that be done?
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.
Is this a list of maps, then? It might be better to have either a list of lists, that is a list of pairs effectively, or a map of lists. That is,
protocol:
- [ http, 80 ]
- [ http, 3128 ]
- [ https, 443 ]
or
protocol:
- http:
- 80
- 3128
- https:
- 443
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.
Of your two, I'd choose the latter -- implied order of an array seems like it will lead to problems. An alternative solution would be to require the admin to list the server twice. I believe the effect of listing multiple ports would be that the server would be in the list twice, and thus get 2x the selections than a server with 1 port. That would be obvious to someone listing the server twice, but, I think not obvious when "just" listing multiple ports.
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 +1 on leaving the protocol as is and just listing a server twice as @mlibbey suggests so that it is obvious to the person configuring this.
6d24a08 to
d00c532
Compare
|
[approve ci] |
This PR adds a YAML parser for the proposed parents.yaml file in PR #3870 under discussion by the nexhop, working group.