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

Exit gracefully when requests encounter a ReadTimeout exception. #1963

Merged
merged 6 commits into from Sep 4, 2015

Conversation

Projects
None yet
4 participants
@shin-
Copy link
Contributor

commented Sep 2, 2015

Fixes #1923

Exit gracefully when requests encounter a ReadTimeout exception.
Signed-off-by: Joffrey F <joffrey@docker.com>
@dnephin

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2015

I think if we're going to add COMPOSE_HTTP_TIMEOUT we need to add it to the docs as well.

What's the advantage over using DOCKER_CLIENT_TIMEOUT ? Is the idea that users can set them to different values if they need to?

@dnephin dnephin added the area/cli label Sep 2, 2015

Document COMPOSE_HTTP_TIMEOUT env config
Signed-off-by: Joffrey F <joffrey@docker.com>
@aanand

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2015

It's more that COMPOSE_HTTP_TIMEOUT is a better variable name, because in general we shouldn't invent DOCKER_* variables - that's the domain of the Docker client.

log.error(
"HTTP request took too long to complete. Retry with --verbose to obtain debug information.\n"
"If you encounter this issue regularly because of slow network conditions, consider setting "
"COMPOSE_HTTP_TIMEOUT to a higher value."

This comment has been minimized.

Copy link
@aanand

aanand Sep 2, 2015

Contributor

"An HTTP request..." reads better, I think.

Would be good to print the current value of the variable here.

@@ -34,5 +34,5 @@ def docker_client():
ca_cert=ca_cert,
)

timeout = int(os.environ.get('DOCKER_CLIENT_TIMEOUT', 60))
timeout = int(os.environ.get('COMPOSE_HTTP_TIMEOUT', os.environ.get('DOCKER_CLIENT_TIMEOUT', 60)))

This comment has been minimized.

Copy link
@aanand

aanand Sep 2, 2015

Contributor

We should deprecate DOCKER_CLIENT_TIMEOUT, i.e. show a deprecation warning if it's set, much as we do with the FIG_* variables:

if 'FIG_PROJECT_NAME' in os.environ:
log.warn('The FIG_PROJECT_NAME environment variable is deprecated.')
log.warn('Please use COMPOSE_PROJECT_NAME instead.')

shin- added some commits Sep 2, 2015

Deprecation warning when DOCKER_CLIENT_TIMEOUT is used
Signed-off-by: Joffrey F <joffrey@docker.com>
Refined error message when timeout is encountered.
Signed-off-by: Joffrey F <joffrey@docker.com>
@shin-

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

How about now? :)

@aanand

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2015

Great. There's now a bit of duplication and inconsistency - the default value is hardcoded twice, and we're only falling back to DOCKER_CLIENT_TIMEOUT in one place, not both.

Could you move it all into one place? Perhaps populate a constant in const.py.

@shin-

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

👍

HTTP_TIMEOUT as importable constant for consistency
Signed-off-by: Joffrey F <joffrey@docker.com>

@shin- shin- force-pushed the shin-:1923-catch-timeout branch from 0066316 to f9c7346 Sep 2, 2015

custom timeout test rewrite
Signed-off-by: Joffrey F <joffrey@docker.com>

@aanand aanand referenced this pull request Sep 4, 2015

Closed

compose scale web=50 fails #1977

aanand added a commit that referenced this pull request Sep 4, 2015

Merge pull request #1963 from shin-/1923-catch-timeout
Exit gracefully when requests encounter a ReadTimeout exception.

@aanand aanand merged commit 10cb0c9 into docker:master Sep 4, 2015

2 checks passed

docker/dco-signed All commits signed
Details
janky Jenkins build Compose-PRs 1255 has succeeded
Details
@aanand

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2015

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.