Skip to content
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 for streams #54

Closed
wants to merge 13 commits into from
Closed

Conversation

rshriram
Copy link

This PR is the counterpart to openresty/stream-lua-nginx-module#30
Its based on @splitice's #25

@agentzh
Copy link
Member

agentzh commented Aug 11, 2016

@rshriram I think you should update .travis.yml in your PR to make the Travis CI tests pass.

Shriram Rajagopalan and others added 2 commits August 14, 2016 09:52
* fix typo in build command

* timeout tests for streams
@rshriram
Copy link
Author

@agentzh I have updated the Travis file. Temporarily it's pointing to my fork of stream lua module. I want the Travis tests to pass before fixing it. I have also temporarily disabled the timeout tests for stream module until the timeout related compilation issue can be resolved.

Should this PR get accepted, when will it be available via standard OpenResty release? I would like to avoid shipping custom variants of OpenResty in my docker images.

@agentzh
Copy link
Member

agentzh commented Aug 15, 2016

@rshriram Just patch the .travis.yml file in your branch to get Travis CI passing.

Yes, they should go into the official OpenResty distribution eventually. But before that, we should do the following:

  1. Merge the corresponding patches into the mainline ngx_stream_lua_module and lua-resty-core.
  2. Bundle the ngx_stream_lua module into the OpenResty distribution.

Let's do the 1st item for now.

upstream backend {
server 0.0.0.1:80;
balancer_by_lua_block {
local b = require "ngx.balancer.stream"
Copy link
Member

@agentzh agentzh Aug 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use ngx.balancer as the API while keeping the implementation specific to the stream subsystem in ngx.balancer.stream. So the user require ngx.balancer instead of ngx.balancer.stream. This encourages Lua code reuse on the user Lua land.

@rshriram
Copy link
Author

One option is to sprinkle a whole bunch of if blocks across the balancer.lua to check if it's in http or stream context (how do I do this?)
I am also getting module not found errors from base.lua. I put stream.lua under the ngx/core/balancer/ folder.

@agentzh
Copy link
Member

agentzh commented Aug 18, 2016

@rshriram No, you can do something like below in the top-level scope of ngx.balancer:

if ngx.config.subsystem == "stream" then
    return require "ngx.balancer.stream"
end

Just one if statement needed ;)

Also check existing test files and Lua modules in lua-resty-core for how to handle Lua module search paths and module names for require properly.

@rshriram
Copy link
Author

Hi @agentzh
I have fixed the tests and added the if check in balancer.lua so that it automatically pulls in ngx.balancer.stream based on the context (as you suggested).
Just FYI: the code will not pass the tests until the corresponding PR in the stream-lua-nginx-module repository is merged.
Also, I had to omit some tests, such as checks for upstream_addr, as those variables are not really available in the stream context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants