-
Notifications
You must be signed in to change notification settings - Fork 199
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
balancer_by_lua #7
Conversation
@splitice Thanks for looking into this. Will you port the tests too? |
I dont have an environment to run them, but a bit of regex would probably do the configuration changes necessary. The error messages would need to be done manually I guess. I'd be happy to do that much. |
@splitice There are perl scripts under |
I'll take a look, its probably not dissimilar to what I whipped up. Currently looking for the cause of a bug, s->connection->log is null in the balancer context causing a segfault when any error occurs or with ngx.log. |
Fix stream session session passing, now it works. Successfully set the current peer to a dynamic value. |
{ | ||
lua_State *L; | ||
ngx_int_t rc; | ||
ngx_stream_session_t *r; |
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.
Need to run the ngx-releng
script to catch obvious NGINX coding style issues like this :) See
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.
Doesn't tell me anything for some reason.
# ./ngx-releng
=======================================
# ls -las src
0 lrwxrwxrwx 1 root root 30 Feb 4 19:37 src -> ../stream-lua-nginx-module/src
# ls -las src/ | head -n 6
total 1064
4 drwxr-xr-x 3 root root 4096 Feb 3 19:14 .
4 drwxr-xr-x 6 root root 4096 Feb 3 19:21 ..
4 drwxr-xr-x 2 root root 4096 Feb 3 19:14 api
4 -rw-r--r-- 1 root root 771 Feb 3 19:13 ddebug.h
16 -rw-r--r-- 1 root root 15790 Feb 4 18:15 ngx_stream_lua_balancer.c
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.
@splitice Found a lot of things on your PR's branch on my side:
agentzh@fedora64 ~/git/stream-lua-nginx-module (master)$ which ngx-releng
~/git/nginx-devel-utils/ngx-releng
gentzh@fedora64 ~/git/stream-lua-nginx-module (master)$ g hub pr-fetch 7
remote: Counting objects: 42, done.
remote: Compressing objects: 100% (42/42), done.
g remote: Total 42 (delta 22), reused 0 (delta 0), pack-reused 0
Unpacking objects: 100% (42/42), done.
From github.com:openresty/stream-lua-nginx-module
* [new ref] refs/pull/7/head -> PR/7
agentzh@fedora64 ~/git/stream-lua-nginx-module (master)$ g co PR/7
Switched to branch 'PR/7'
agentzh@fedora64 ~/git/stream-lua-nginx-module (PR/7)$ ngx-releng
=======================================
src/ngx_stream_lua_balancer.h
14:ngx_int_t ngx_stream_lua_balancer_handler_inline(ngx_stream_session_t *s, ngx_log_t *r,
17:ngx_int_t ngx_stream_lua_balancer_handler_file(ngx_stream_session_t *s, ngx_log_t *r,
src/ngx_stream_lua_balancer.c
174: p = ngx_copy(p, NGX_STREAM_LUA_INLINE_TAG, NGX_STREAM_LUA_INLINE_TAG_LEN);
220: bp = ngx_pcalloc(r->connection->pool, sizeof(ngx_stream_lua_balancer_peer_data_t));
333:ngx_stream_lua_balancer_by_chunk(lua_State *L, ngx_log_t *log, ngx_stream_session_t *s)
src/ngx_stream_lua_common.h
118:typedef ngx_int_t (*ngx_stream_lua_srv_conf_handler_pt)(ngx_stream_session_t *s, ngx_log_t *log,
src/ngx_stream_lua_balancer.c
547: //ngx_stream_upstream_state_t *state;
src/ngx_stream_lua_phase.c
60:
61:
src/ngx_stream_lua_balancer.c
532:
src/ngx_stream_lua_module.c
107:
src/ngx_stream_lua_common.h
117:
165:
232:
src/ngx_stream_lua_balancer.c
533: *err = NULL;
544: /* NOT PORTED: IS IT POSSIBLE? */
src/ngx_stream_lua_common.h
233: struct {
t/123-lua-path.t
19:master_on();
t/servroot/logs/error.log
t/cert/equifax.crt
t/cert/startcom.crt
src/api/ngx_stream_lua_api.h
22:#define ngx_stream_lua_version 1
src/ngx_stream_lua_phase.c:
found unnecessary tail space
60:
found unnecessary tail space
61:
src/ngx_stream_lua_balancer.c:
incorrect front space, unclosed bracket
60: lscf->balancer.src.data,
incorrect front space, unclosed bracket
80: lscf->balancer.src.data,
variable name should align with the previous line
121: ngx_stream_lua_srv_conf_t *lscf = conf;
incorrect front space, unclosed bracket
144: value[1].len);
variable name should align with the previous line
249: ngx_stream_session_t *r;
variable name should align with the previous line
413: ngx_stream_lua_ctx_t *ctx;
found unnecessary tail space
532:
src/ngx_stream_lua_module.c:
found unnecessary tail space
107:
src/ngx_stream_lua_common.h:
found unnecessary tail space
117:
found unnecessary tail space
165:
found unnecessary tail space
232:
incorrect front space, level indent
234: ngx_str_t src;
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.
@splitice Are you sure your nginx-devel-utils clone is up to date? Also, better add the path of your nginx-devel-utils directory to your PATH environment.
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.
@splitice The git master is clean BTW:
agentzh@fedora64 ~/git/stream-lua-nginx-module (master)$ ngx-releng
=======================================
t/123-lua-path.t
19:master_on();
t/servroot/logs/error.log
t/cert/equifax.crt
t/cert/startcom.crt
src/api/ngx_stream_lua_api.h
22:#define ngx_stream_lua_version 1
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.
Its the latest, I just cloned it now.
I just created a symlink to src in the directory out of lazyness so the scripts are in the cwd/pwd.
…ithout --with-debug. thanks Mathew Heard for the report in #7.
Rebase against upstream
…rn status 0 always.
There is a bunch of style fixes |
@splitice The |
@splitice Thanks for your quick updates. @doujiang24 Please have a look at this PR when you have a chance. Thanks! |
@splitice |
This is a pull request for the code. |
@splitice |
Its in my branch - https://github.com/splitice/stream-lua-nginx-module |
@splitice |
@splitice |
@daxiong380380 balancer_by_lua* is officially documented in lua-nginx-module: https://github.com/openresty/lua-nginx-module#balancer_by_lua_block What is implemented here should be (mostly) compatible. |
@agentzh any ETA on when this PR will be merged? What needs to be done to get this merged into upstream? I can try to help, if I manage to get my head around the nginx module stuff. |
int *status, char **err) | ||
{ | ||
|
||
// This is not yet implemented. |
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.
Style: bad indentation and the C++-style line comments should be avoided.
This needs to be rebased to the latest master branch before it can get merged. Also it has various coding style issues that need to be fixed before the merge. |
@rshriram No ETA yet since it has unresolved issues (see my comments above). |
We need to pass as many test cases as possible from ngx_http_lua_module for balancer_by_lua* and ngx.balancer here. |
I currently do not have any time to spend on this patch. PR's welcome to my fork. I am happy to re-base, but without style work its frivolous currently. I just don't have time for that currently. |
@splitice @agentzh I have fixed all the styling issues.. and my branch is here: https://github.com/rshriram/stream-lua-nginx-module |
No concerns on my end, do whichever is easiest / Github allows :) |
Closed in favour of further work by @rshriram |
Here is my first attempt at porting balancer_by_lua from the http module to stream.
TODO:
ngx_stream_lua_ffi_balancer_get_last_failure
, if its possible?If you have any comments, please dont hesitate. This is an early pull request to facilitate that discussion. I have tried to make it a direct conversion, and used a few string replacements where possible, some comments are hence a bit silly.
I have also thrown
#if (NGX_DEBUG)
around the unused ngx_connection_t's that where annoying me with errors (and with the right flags, warnings). I can take them out, but I guess they are a plus?