feature: add the balancer.recreate_request function, which allows user to recreate request buffer in balancer phase. #302
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Member
| @dndx just a reminder, the CI failed. |
dndx force-pushed the proxy_recreate_request branch from ef551a4 to 20204f1 Compare 
Member Author
| @doujiang24 Yes the test plan was wrong. I have fixed the number. |
dndx force-pushed the proxy_recreate_request branch from 20204f1 to 5a27cf2 Compare dndx added a commit to Kong/kong-build-tools that referenced this pull request Jun 30, 2020
…ocao These will be removed once openresty/lua-nginx-module#1734 and openresty/lua-resty-core#302 are merged
dndx added a commit to Kong/kong-build-tools that referenced this pull request Jul 1, 2020
…ocao These will be removed once openresty/lua-nginx-module#1734 and openresty/lua-resty-core#302 are merged
Member Author
| Ping @doujiang24. Could you give another look at this? Thanks! |
thibaultcha requested changes Jul 16, 2020
lib/ngx/balancer.lua Outdated
| long read_timeout, char **err); | ||
| | ||
| int | ||
| ngx_http_lua_ffi_balancer_recreate_request(ngx_http_request_t *r, |
Member
There was a problem hiding this comment.
style: FFI prototype declarations should have the return type on the same line, like in the C module
lib/ngx/balancer.md Outdated
| [Back to TOC](#table-of-contents) | ||
| | ||
| recreate_request | ||
| ------------ |
Member
There was a problem hiding this comment.
style: this line should underline the whole method name
lib/ngx/balancer.md Outdated
| | ||
| **context:** *balancer_by_lua** | ||
| | ||
| Regenerates the request buffer for sending to the upstream server. This is useful, for example |
Member
There was a problem hiding this comment.
Maybe better to stay consistent with wording? i.e. use "recreate" instead of "regenerate" which is the terminology used everywhere else when referring to this API.
lib/ngx/balancer.md Outdated
| | ||
| Normally this does not work because the request buffer is created once during upstream module | ||
| initialization and won't be regenerated for subsequent retries. However you can use | ||
| `proxy_set_header My-Header $my_header` and sets the `ngx.var.my_header` variable inside the |
lib/ngx/balancer.md Outdated
| Normally this does not work because the request buffer is created once during upstream module | ||
| initialization and won't be regenerated for subsequent retries. However you can use | ||
| `proxy_set_header My-Header $my_header` and sets the `ngx.var.my_header` variable inside the | ||
| balancer phase. Calling this function after that will cause the request buffer to be re-generated |
Member
There was a problem hiding this comment.
"Calling this function after that" can be confusing for the reader. "Calling balancer.recreate_request() after updating a header field will cause..."
lib/ngx/balancer.md Outdated
| and the `My-Header` header will thus contain the new value. | ||
| | ||
| **Warning:** because the request buffer has to be recreated and such allocation occurs on the | ||
| request memory pool, the old buffer has to be thrown away and only be freed after the request |
Member
There was a problem hiding this comment.
and will only be freed when...
to recreate request buffer in balancer phase. This allows certain request parameters, such as headers (including `Host` header) to be modified between balancer retries.
dndx force-pushed the proxy_recreate_request branch from 5a27cf2 to 1e749c8 Compare Member Author
| Thanks for the reviews @thibaultcha, I fixed the styling and ran the document through with |
doujiang24 approved these changes Jul 18, 2020
syzh approved these changes Jul 19, 2020
agentzh approved these changes Jul 20, 2020
agentzh requested changes Jul 20, 2020
lib/ngx/balancer.md Outdated
| to this function should be made **only** if you know the request buffer must be regenerated, | ||
| instead of unconditionally in each balancer retries. | ||
| | ||
| This function was first added in the `0.1.19` version of this library. |
Member
There was a problem hiding this comment.
This should be changed to 0.1.20.
agentzh requested changes Jul 20, 2020
t/balancer.t Outdated
| print("here") | ||
| local b = require "ngx.balancer" | ||
| | ||
| if ngx.ctx.balancer_ran then |
Member
There was a problem hiding this comment.
I think balancer_run is more grammatically correct.
doujiang24 pushed a commit that referenced this pull request Jul 30, 2020
…ser to recreate request buffer in balancer phase. (#302) This allows certain request parameters, such as headers (including `Host` header) to be modified between balancer retries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
I hereby granted the copyright of the changes in this pull request
to the authors of this lua-resty-core project.
Feature was originally developed by @locao inside Kong/lua-kong-nginx-module#13. With slight modifications from @dndx.
Related PR: openresty/lua-nginx-module#1734.