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

Pull request 269 revisited #314

Merged
merged 1 commit into from
Jun 23, 2014
Merged

Pull request 269 revisited #314

merged 1 commit into from
Jun 23, 2014

Conversation

janorn
Copy link
Contributor

@janorn janorn commented May 15, 2014

I have rebased Pull request 269.
This patch is now reduced to adding the following parameters:
client_body_temp_path
http_tcp_nodelay
http_tcp_nopush
keepalive_timeout
logdir
mail
pid
proxy_temp_path
run_dir
sendfile
spdy
temp_dir

@jfryman
Copy link
Contributor

jfryman commented May 16, 2014

Looks like it needs another rebase... :/

@janorn
Copy link
Contributor Author

janorn commented May 16, 2014

Yet another try!

@jfryman
Copy link
Contributor

jfryman commented May 21, 2014

This is really super. I'd love it if another community member would also look and give a 👍 before this merges, since it is quite a large change.

@janorn
Copy link
Contributor Author

janorn commented May 21, 2014

I am testing it a bit more at the moment. I expect to submit som small fixes shortly.

@janorn
Copy link
Contributor Author

janorn commented May 28, 2014

Added for more config options to remove all references to params.
client_body_buffer_size
proxy_connect_timeout
proxy_read_timeout
proxy_send_timeout

This resolved and issue with proxy_read_timeout from previous patches.

@janorn
Copy link
Contributor Author

janorn commented May 28, 2014

This is becoming a big patch so I don't know how we take this further.

@janorn
Copy link
Contributor Author

janorn commented May 28, 2014

Rebased pull request.

@janorn
Copy link
Contributor Author

janorn commented Jun 4, 2014

It is becoming a bit tedious to rebase all the time. Can we do something different?
Shall I try to break this pull request into pieces?!

@jfryman
Copy link
Contributor

jfryman commented Jun 4, 2014

Yeah, I was thinking the same thing earlier as well. It's just too big and keeps getting hit by the blast radius of other pulls.

@janorn
Copy link
Contributor Author

janorn commented Jun 4, 2014

Slightly thinner. Will work on breaking out some more.

@saz
Copy link
Sponsor Contributor

saz commented Jun 5, 2014

Any help required on this? There are some changes I need and I'd love to see this merged.

@janorn
Copy link
Contributor Author

janorn commented Jun 5, 2014

I am shrinking this pull request by breaking out parts of it. Feel free to take a stab on it and take some options and create a new pull request. I will rebase when they have been merged and I guess at one stage this request will be small enough ;-)

@janorn
Copy link
Contributor Author

janorn commented Jun 12, 2014

I have rebased this again. How much smaller do you want me to make it? Some of these variable changes touches a lot of code.

@janorn
Copy link
Contributor Author

janorn commented Jun 16, 2014

This patch is now reduced to adding the following parameters:
client_body_temp_path
http_tcp_nodelay
http_tcp_nopush
keepalive_timeout
logdir
mail
pid
proxy_temp_path
run_dir
sendfile
spdy
temp_dir

- make many more things configurable
- stop using ::params::* for things that are configurable
- add worker_rlimit_nofile option
- add tcp_nopush option
@saz
Copy link
Sponsor Contributor

saz commented Jun 23, 2014

👍

@jfryman
Copy link
Contributor

jfryman commented Jun 23, 2014

Alright. Looks alright to me, and tests pass. Let's go with it. Many ❤️ for working on this!

jfryman pushed a commit that referenced this pull request Jun 23, 2014
Pull request 269 revisited
@jfryman jfryman merged commit 09b42b9 into voxpupuli:master Jun 23, 2014
@janorn
Copy link
Contributor Author

janorn commented Jun 23, 2014

Thanks! Now I can concentrate on other changes ;-)

@janorn janorn deleted the pull_269 branch June 23, 2014 19:33
@janorn
Copy link
Contributor Author

janorn commented Jun 23, 2014

I guess PR #269 could be closed now.

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.

4 participants