Prevent duplicate attempts to start Ryuk container #2245
Conversation
| @@ -17,9 +17,6 @@ | |||
| import lombok.SneakyThrows; | |||
| import lombok.Synchronized; | |||
| import lombok.extern.slf4j.Slf4j; | |||
| import org.hamcrest.BaseMatcher; | |||
| import org.hamcrest.Description; | |||
| import org.rnorth.visibleassertions.VisibleAssertions; | |||
There was a problem hiding this comment.
Using VisibleAssertions for the logging in checks seems like major overkill, so I've changed it to use simple log statements.
| // fail-fast if checks have failed previously | ||
| if (cachedChecksFailure != null) { | ||
| throw cachedChecksFailure; | ||
| } |
There was a problem hiding this comment.
It seems like a bug that currently checks can fail one time and then even try to run the checks again. We shouldn't; we should be forcing a failure so that the check failure gets investigated (rather than just the first test failing).
| } | ||
| ); | ||
| } | ||
| } catch (RuntimeException e) { |
There was a problem hiding this comment.
WDYT about catching any Exception, not just RuntimeException?
There was a problem hiding this comment.
Well, unless there's a naughty @SneakyThrows somewhere, it doesn't look like any checked exceptions are thrown. Also, if we catch an Exception we're in a situation where the client() method would have to be able to thow Exception. We could wrap a cached Exception inside a RuntimeException. But this feels a bit excessive.
There was a problem hiding this comment.
I am just cautious about the future changes where we may introduce a checked exception that is thrown but not stored to be re-thrown later
There was a problem hiding this comment.
@SneakyThrows is another good point, yes
There was a problem hiding this comment.
Perhaps the good news is that a newly thrown checked exception would force us to catch it (client doesn't declare throws) - unless we use @SneakyThrows a lot more, which I think we should try and avoid. 😀
If for example, checks fail once but the Docker client is otherwise initialized, an error would occur due to an attempt to start a Ryuk container twice with the same name. This changed is aimed at avoiding that situation. Additionally, if checks fail once they should fail on every subsequent attempt to start a client. Therefore, we cache the failure and rethrow it.
rnorth
force-pushed
the
prevent-ryuk-double-start
branch
from
d23519d
to
63392d9
Compare
…s `BadRequestException`
Prevent duplicate attempts to start Ryuk container If for example, checks fail once but the Docker client is otherwise initialized, an error would occur due to an attempt to start a Ryuk container twice with the same name. This changed is aimed at avoiding that situation. Additionally, if checks fail once they should fail on every subsequent attempt to start a client. Therefore, we cache the failure and rethrow it. Co-authored-by: Sergei Egorov <bsideup@gmail.com>
If for example, checks fail once but the Docker client is otherwise initialized, an error would occur due to an attempt to start a Ryuk container twice with the same name.
This changed is aimed at avoiding that situation.
Additionally, if checks fail once they should fail on every subsequent attempt to start a client. Therefore, we cache the failure and rethrow it.