- run 'stopContainer' in a Future (resolves #518) #528
Conversation
Signed-off-by: Jae Gangemi <[email protected]>
jgangemi
changed the title
|
@rhuss i'd appreciate consideration (and a release) at your earliest convenience. this shaved 10 minutes off our build. |
|
ok, will have a look tomorrow. The only possible problem I see here is, that I already have concurrency issues with Docker daemon running over unix socket (when waiting to a log output in a different thread), so I'm afraid that for this use case there might even more serious issues. However, for TCP based connection everything should be fine. |
|
yeah - unix socket handling is crappy in general. i haven't looked at any of the libraries i saw to see if they were better then what we are using. thinking about it now, it might not even need to run in a maybe we could add a |
|
oops, I mixed this up with #531 (where I see more serious concurrency issue coming). Guess the future is more or less sage ;-). to be continued tomorrow ... |
| @@ -25,6 +25,8 @@ | |||
| */ | |||
| public class WaitUtil { | |||
|
|
|||
| private static final int DEFAULT_DOCKER_TIMEOUT = 10; | |||
There was a problem hiding this comment.
10s maybe a bit high as default, which is even used when no killGracePeriod is specified. In fact I wouldn't wait at all if no killGracePeriod is specified so simply skipping any wait when int wait is null.
Any particular reason why you always want a wait when a shutdown happens ?
There was a problem hiding this comment.
i don't always want to wait - the docker container takes up to 10 seconds which is why i used that default. if it exited before then, the future would complete, so i don't think we were waiting any extra time w/ this.
but whatever you think is best here.
There was a problem hiding this comment.
yeah - looking at your change, i'm not 100% sure that's correct either in terms of returning wait = 0 b/c you are always waiting some time for the daemon to complete, so i actually think the original implementation was correct.
get(long timeout, TimeUnit unit)
Waits if necessary for at most the given time for the computation to complete, and then retrieves its result, if available.
if anything maybe it should be if (wait == 0) { wait = DEFAULT } (i can't remember what i originally had and i'm about to walk out the door for something) b/c then we are matched against the daemon itself.
and now that i think about it, maybe that value should be padded by a second or two so we're sure we get the output back from the docker daemon. i think the only time there would be 'extra' waiting is if the docker daemon dies, and at that point you've got bigger problems.
|
LGTM except for the default value. If don't mind I'll fix that right now and merge to integration. We can still discuss this till tomorrow. You can expect a release tomorrow or the day after. |
resolves #518, now the plugin will continue as soon as the container stops, hurray faster builds!