Hi Alan,
Thanks for comments,
On 11/9/2015 6:20 AM, Alan Bateman wrote:
On 05/11/2015 21:56, Roger Riggs wrote:
Please review the new ProcessBuilder.startPipeline API,
implementation, and tests.
:
javadoc of ProcessBuilder: only startPipeline is new:
http://cr.openjdk.java.net/~rriggs/pipedoc/
I skimmed over the javadoc and it looks quite good.
The method name seems okay, I initially assumed it would be something
like startProcesses but that doesn't convey how they are arranged.
Using varargs seem reasonable for the examples like you have in the
javadoc but there may be other usages where this might be annoying.
Returning List<Process> seems okay. The last Process is of course the
most interesting but having some way to get the exitValue of the
intermediate processes will be important for logging and
troubleshooting purposes.
For testing purposes then I assume the javadoc needs to make it clear
that the returned list is the same size as the input array and that
the Process at index i corresponds to ProcessBuilder i.
good point.
Did you consider ignoring the redirects of the intermediate processes
so that IAE doesn't need to be thrown?
That might be a plus if a ProcessBuilder is being reused in different
contexts bot in a out of a pipeline.
But it also might lead to confusing bugs because the internal redirect
settings are being silently ignored.
It seemed less surprising to flag it as a coding error and throw an
exception.
I assume the method needs to specify SecurityException and other
exceptions that start can throw.
yes, is it better to specify it as a delegation to the start() method
or to repeat all of the
specification of start?
On the clean-up then do we need to reassert the interrupt status if
interrupted?
yes, fixed.
Thanks, Roger
-Alan.
