-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about catching any Exception
, not just RuntimeException
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SneakyThrows
is another good point, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.