-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: (1 of 2) Supporting changes for upstream example module. #36
Conversation
src/http/upstream.rs
Outdated
/// opportunity to set custom data. | ||
/// Load Balancing: <https://nginx.org/en/docs/dev/development_guide.html#http_load_balancing> | ||
#[macro_export] | ||
macro_rules! http_upstream_peer_init { |
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.
really hard to come up with a name of that macro, as it takes two args: request and an upstream config and then it is update a handler bod.
this macro itself has no mention of peer or init. probably this can be also a good candidate to be outside of the ngx crate and be just added to the module implementation itself.
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.
For these functions, the upstream modules all use this format: ngx_http_upstream_init_[MODULE_NAME]_peer
I kind of followed the typedef for the signature, but accidentally transposed peer and init. We can name it identical to that typedef (minus the ngx, this repo tends to omit that prefix).
I also placed it in upstream.rs because the typedef is in ngx_http_upstream.h, so it's a clue and alignment to the original.
If we were more faithful to the NGINX name it would be http_upstream_init_peer_pt
. I think keeping this in upstream is a helpful alignment to NGINX code base.
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.
Renamed to reflect the function type it defines.
4ae3f5b
to
798ff75
Compare
Hi, Is there any timeline on when the upstream module will be available? |
Working on it now, traveling next week, but working to close this review and then publish the module next. |
- functions to get mut and const module context from the ngx_http_upstream_srv_conf_t - Request from trait - Request upstream getter - Request peer init macro
9f5859b
to
5b420fc
Compare
Name macro to reflect underlying NGINX type
5b420fc
to
e202d03
Compare
Backed out of adding Upstream object for now. Current commits build and pass lint without warnings. |
Previous logging changes are moved to #40
Fixes #30 (replaces PR #35 )
Checklist
Before creating a PR, run through this checklist and mark each as complete.