Combine DB container tests into single module, and improve error handling/display #243
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rnorth
force-pushed
the
reduce-error-output
branch
from
184b690
to
1ce1d9f
Compare
bsideup
reviewed
Nov 18, 2016
| @@ -815,6 +815,10 @@ public ExecResult execInContainer(Charset outputCharset, String... command) | |||
|
|
|||
| } | |||
|
|
|||
| if (!isRunning()) { | |||
| @@ -67,6 +68,8 @@ public void registerContainerForCleanup(String containerId, String imageName) { | |||
| */ | |||
| public void stopAndRemoveContainer(String containerId) { | |||
| stopContainer(containerId, registeredContainers.get(containerId)); | |||
|
|
|||
| registeredContainers.remove(containerId); | |||
There was a problem hiding this comment.
maybe we should do it in finally {} block?
There was a problem hiding this comment.
Actually even though it's a little brittle I'd prefer to keep it that way. If stopContainer fails for some reason, my feeling is that it's slightly better to potentially try again by leaving the containerId in the registered set.
| @@ -83,6 +86,13 @@ public void stopAndRemoveContainer(String containerId, String imageName) { | |||
|
|
|||
| private void stopContainer(String containerId, String imageName) { | |||
|
|
|||
| List<Container> allContainers = dockerClient.listContainersCmd().withShowAll(true).exec(); | |||
|
|
|||
| if (allContainers.stream().noneMatch(it -> it.getId().equals(containerId))) { | |||
There was a problem hiding this comment.
just FYI:
allContainers.stream().map(Container::getId).noneMatch(containerId::equals)
| @@ -106,6 +116,13 @@ private void stopContainer(String containerId, String imageName) { | |||
| } | |||
|
|
|||
| try { | |||
| InspectContainerResponse containerInfo = dockerClient.inspectContainerCmd(containerId).exec(); | |||
There was a problem hiding this comment.
we don't use containerInfo anywhere
rnorth
force-pushed
the
reduce-error-output
branch
5 times, most recently
from
4164e57
to
381d9c7
Compare
Closed
…r refactoring of common elements. Adopt MySQL and MariaDB default configuration with lighter resource requirements
…o enable them to be volume mounted into containers.
…path resources that have been extracted from JAR files.
* ensure that liveness `exec` calls don't fire when a container is not running * check that containers exist before trying to clean up in ResourceReaper * filter out common ANSI control code in stderr output * amend frame consumer result callback to suppress errors
…when DB containers are slow to start
rnorth
force-pushed
the
reduce-error-output
branch
from
a29d49e
to
408e983
Compare
rnorth
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR was created on top of #242, and should be merged after that.This change includes what was previously covered by #242: Refactoring of all DB container tests into a single separate module. The idea is that this will make it easier to reduce duplication between tests in the future. Along the way, some reliability issues with DB containers have been worked on:
This also goes through a few places where unnecessary stack traces or errors were indicated during tests. It emerged that some behaviour (container reaper and exec-based liveness checks) were running against stopped or deleted containers, causing loud error logging from docker-java.