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

better handling of Docker timeouts #865

Merged
merged 4 commits into from Feb 8, 2016
Merged

better handling of Docker timeouts #865

merged 4 commits into from Feb 8, 2016

Conversation

@ssalinas
Copy link
Member

ssalinas commented Jan 26, 2016

@wsorenson this was my addition to #864

DockerUtils has a single threaded ExecutorService (we only need one call at a time right now anyways) that will execute each with a timeout. So, anything docker is not executed on the main thread that needs to keep going in order for the Executor to communicate with mesos.

Also adds a default thread pool of 5 for the docker client. The initial client was meant for managing large numbers of containers concurrently, thus the limit of 100.

@ssalinas ssalinas added the hs_staging label Jan 27, 2016
@ssalinas ssalinas added the hs_qa label Jan 27, 2016
throw new ProcessFailedException(String.format("Timed out trying to reach docker daemon after %s seconds", configuration.getDockerClientTimeLimitSeconds()));
dockerUtils.pull(task.getTaskInfo().getContainer().getDocker().getImage());
} catch (DockerException e) {
throw new ProcessFailedException(String.format("Could not pull docker image due to error: %s", e));

This comment has been minimized.

Copy link
@wsorenson

wsorenson Jan 27, 2016

Contributor

Don't add the exception to an error message, include the exception as a cause argument.

try {
return callWithTimeout(callable);
} catch (Exception e) {
LOG.error("Caught exception while getting container status", e);

This comment has been minimized.

Copy link
@wsorenson

wsorenson Jan 27, 2016

Contributor

I would only log all of these if you don't control the code that is going to handle the thrown exception - meaning, you need to ensure it gets logged here because you don't know if it will be logged when caught.

@tpetr tpetr modified the milestone: 0.4.9 Jan 27, 2016
@ssalinas ssalinas added the hs_stable label Feb 1, 2016
@tpetr tpetr modified the milestones: 0.4.9, 0.4.10 Feb 2, 2016
ssalinas added a commit that referenced this pull request Feb 8, 2016
better handling of Docker timeouts
@ssalinas ssalinas merged commit 1f2d563 into master Feb 8, 2016
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
@ssalinas ssalinas deleted the docker_threading branch Feb 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.