Exec.exec returning Future and hiding Process as an implementation detail #1322
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @netikras! |
k8s-ci-robot
added
the
size/L
|
I signed it |
k8s-ci-robot
added
cncf-cla: yes
|
|
|
You need to run |
| package io.kubernetes.client.custom; | ||
|
|
||
| import java.io.InputStream; | ||
| import java.util.concurrent.*; |
There was a problem hiding this comment.
No wildcard imports please.
| import java.io.InputStream; | ||
| import java.util.concurrent.*; | ||
|
|
||
| public class AsyncPump implements Runnable { |
There was a problem hiding this comment.
Quick JavaDoc for the use of this class.
| } | ||
|
|
||
| public AsyncPump stop() { | ||
| if (thread != null) |
There was a problem hiding this comment.
Curly braces around all conditions.
| consumer.accept(giveAway); | ||
| } | ||
| onFinish.run(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Catch specific exceptions.
| } catch (Exception e) { | ||
| try { | ||
| errorHandler.accept(e); | ||
| } catch (Exception exception) { |
| promise.complete(null); | ||
| } | ||
|
|
||
| public interface NoisyConsumer<T> { |
There was a problem hiding this comment.
What is the purpose of these interfaces?
| import java.io.Reader; | ||
| import java.io.UnsupportedEncodingException; | ||
|
|
||
| import java.io.*; |
There was a problem hiding this comment.
As above no wildcard imports please.
|
Absolutely. Done |
| io.setStdout(process.getInputStream()); | ||
| io.setStderr(process.getErrorStream()); | ||
| Thread processWaitingThread = | ||
| new Thread( |
There was a problem hiding this comment.
This method is going to be hard to test because of this. Can you use an ExecutorService instead? That can be injected in the test.
There was a problem hiding this comment.
Sure thing!
private void runAsync(String taskName, Runnable task) {
if (this.executorService == null) {
Thread thread = new Thread(task);
thread.setName(taskName);
thread.start();
} else {
executorService.submit(task);
}
}
Will this cut it? Using an ExecutorService if there is supplied one, and falling back to standalone Thread if there isn't one. And a simple Exec.setExecutorService() to supply the executor. Although the executor would also be used in this sole method (unless reused somewhere else later on).
I really don't feel like adding yet another parameter to the method, since there already are 8+ and the ES would be mostly optional.
Another option - use a protected field. or even a protected runAsync(String taskName, Runnable task){...}, which could be overridden in tests. Which approach would you prefer?
There was a problem hiding this comment.
This approach is fine. Thanks!
There was a problem hiding this comment.
which is "this"? :)
I have gone with the protected runAsync(String taskName, Runnable task){...} (seemed more appropriate). Shall I leave it or replace it with the code block in my comment above?
| @@ -47,6 +54,7 @@ | |||
| private static final Logger log = LoggerFactory.getLogger(Exec.class); | |||
|
|
|||
| private ApiClient apiClient; | |||
| private long graceTimeoutMs = 5_000L; | |||
There was a problem hiding this comment.
Given that this is only used in one method, make this a parameter instead? (or delete)
There was a problem hiding this comment.
I'll delete it then.
| log.warn("Process timed out after {} ms. Shutting down: {}", timeoutMs, cmdStr); | ||
| process.destroy(); | ||
| try { | ||
| exited = process.waitFor(graceTimeoutMs, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
It's a little weird to wait after the user specified timeout. I think we should delete all of the grace period logic. The user should just pass in whatever timeout they want.
There was a problem hiding this comment.
Have you ever closely monitored a Tomcat process shutting down? :) It takes a few seconds to clear all the pools, close all the connections, commit all the remaining transactions after the JVM receives a signal #15. Similar delays are observed in multi-classloader applications (e.g. osgi-based). Shutting down the application gracefully might take a tad more than just a heartbeat.
In line 303 I'm (hopefully) sending a sigterm, so the application starts its shutdown sequence. Some applications might take considerable amounts of time to shutdown (e.g. a JVM with a debugger attached), thus I cannot guarantee the process will be stopped immediately after the timeout in line #295. Telling the caller the process has stopped could lead to resources' exhaustion in some scenarios (i.e. BUGS). So I'm giving the process a chance to finish its business itself in 5 seconds (by default). If it fails to do so (could be many reasons), then I'm using my last card - sending (hopefully) a sigkill.
Sending a sigkill must always be the last resort.
I'd really like to keep the grace period in the code. I can hardcode the grace period if you'd like me to (could be extracted later on if required), but I'd sleep better knowing I've pushed the correct process termination procedure to a public repo :)
There was a problem hiding this comment.
But this isn't a real process, it's a remote process running on a different service in a container on the cluster, so I don't think that stuff applies here.
There was a problem hiding this comment.
That's the obvious consequence of Liskov's principle violation :) Frankly, I don't know what does this client does when I ask it to terminate. So which one does reliably destroy the connection and remote process? destroy() or destroyForcibly()? I'll use the one you name and remove the grace period then.
There was a problem hiding this comment.
destroy() does the right thing here (it's not 100% 'reliable' in the sense that nothing in distributed systems is 100% "reliable"), but destroy closes the web socket connection which should have the net effect of stopping the process on the remote server (eventually), but from the client's perspective, it's terminated once the socket is closed.
Code is here:
https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/Exec.java#L476
|
This looks good to me, can you add a unit test and squash the commits? Then it will be ready to merge. Thanks! |
k8s-ci-robot
added
size/XL
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: netikras, yue9944882 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
as per #1310