-
Notifications
You must be signed in to change notification settings - Fork 237
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
#711 Make service infra startup timeout configurable #724
Conversation
# (i.e. Squbs listeners). The default timeout is (number of listeners * 10) seconds. This timeout | ||
# applies to both the wait time for a response from each listener and the overall time it takes for | ||
# all listeners to start. | ||
# service-infra-timeout = 1 minute |
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.
Is this line service-infra-timeout = 1 minute
supposed to be commented out?
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.
Yes, in order to preserve existing behavior, which is to have the timeout be governed by the number of listeners. Your question is making me think about the doc effort though. I may put something in the timeout error message about this config).
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.
Typically the reference.conf
is for capturing the default settings here. "over-rides" of the default are typically in application.conf
. Also I believe having this timeout value be both the per listener timeout as well as the total timeout might be confusing to users.
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.
I changed it to have 2 timeouts and the values are not commented out.
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.
LGTM
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.
LGTM
Tests and documentation TBD.