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

[JENKINS-55392] Do not close Kubernetes client after containerLog step #435

Merged
merged 1 commit into from
Mar 4, 2019

Conversation

carlossg
Copy link
Contributor

as it is being cached and reused

as it is being cached and reused
@carlossg carlossg changed the title [JENKINS-55392] Do not close Kubernetes client [JENKINS-55392] Do not close Kubernetes client after containerLog step Feb 27, 2019
@carlossg
Copy link
Contributor Author

this may be enough and not need #418

Copy link
Member

@Vlatombe Vlatombe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@carlossg carlossg merged commit ccd5079 into master Mar 4, 2019
@carlossg carlossg deleted the JENKINS-55392-2 branch March 4, 2019 09:23
.expireAfterWrite(CACHE_EXPIRATION, TimeUnit.SECONDS)
.newBuilder() //
.maximumSize(CACHE_SIZE) //
.expireAfterWrite(CACHE_EXPIRATION, TimeUnit.SECONDS) //
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

@@ -107,7 +107,7 @@ public boolean start() throws Exception {
@Override
public void stop(@Nonnull Throwable cause) throws Exception {
LOGGER.log(Level.FINE, "Stopping container step.");
closeQuietly(getContext(), client, decorator);
closeQuietly(getContext(), decorator);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW I would recommend getting rid of Resources.closeQuietly and switch to Java 7 suppressed exceptions—unless you have a specific reason to expect an exception from some close method which you have a specific reason to want to recover from, in which case write that explicitly. But the default should be to propagate failures.

Also I recommend all StepExecution subtypes delegate to super.stop unless you really know what you are doing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants