-
Notifications
You must be signed in to change notification settings - Fork 32
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
Allow nginx to start even if resolution failed #12
base: master
Are you sure you want to change the base?
Conversation
Just cosmetic but helps to understand the code.
If nginx can't access the resolver during startup for any reason, do not prevent nginx from starting and let it recover later. Also fixes a crash in case the resolution would suddently return no entities.
Are you still maintaining this module? |
@rs I think I have what might be a better solution to the problem.
I think this approach is a bit better/cleaner than both the original and your versions:
|
if do not prevent nginx from starting , all request to upstream will fail.i don't think its a good idea. |
@wdaike imo there's no practical difference whether requests fail at startup or at some later stage in case the DNS server becomes temporarily unavailable. On the other hand, a service that fails to startup due to a temporary error can cause a lot of maintenance trouble. @rs Please see https://github.com/viciious/ngx_upstream_jdomain/tree/lazy-startup |
@wdaike I can see how this might be a bad idea in case of a restart though. Perhaps we should add a config option to allow tolerating name resolution failures at startup then. |
Added a "no_fail" option which defaults to 0. Turning the option on allows nginx to startup even in case of domain name resolution failure. https://github.com/viciious/ngx_upstream_jdomain/commits/lazy-startup @wdaike If that's ok with you, i'll make a new pull request which will supersede this one. |
If nginx can't access the resolver during startup for any reason, do not prevent nginx from starting and let it recover later.
Also fixes a crash in case the resolution would suddently return no entities.
(This PR is based on #11)