----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69408/#review210855 -----------------------------------------------------------
Thanks for the works, it's a very neat improvement to SSH action. In general, a nice approach and pretty good coverage. Most of my comments point towards better readability / maintainability. I'd extend the test coverage by adding: * slow processes that drain up till several seconds * processes that don't drain on a linear scale, but after a while streams get paused and then resumed w/ random pause times * drain a couple of processes at a time in different threads, and see whether all of those finish correctly * randomly failing while draining some parallel processes core/src/main/java/org/apache/oozie/util/BufferDrainer.java Lines 44-45 (patched) <https://reviews.apache.org/r/69408/#comment295616> A few thoughts on parameters etc.: * `inputBuffer` and `errorBuffer` should not be inout parameters, but instance variables. There should be a `getInputBuffer()` / `getErrorBuffer()` methods that throw `IllegalStateException` when `drainBuffers()` hasn't yet been finished (not called or running) * `p` and `maxLength` could be instance variables as well, initialized as constructor parameters * this method should not be `static` core/src/main/java/org/apache/oozie/util/BufferDrainer.java Lines 68 (patched) <https://reviews.apache.org/r/69408/#comment295617> Dead code core/src/main/java/org/apache/oozie/util/BufferDrainer.java Lines 71 (patched) <https://reviews.apache.org/r/69408/#comment295618> Dead code core/src/main/java/org/apache/oozie/util/BufferDrainer.java Lines 82 (patched) <https://reviews.apache.org/r/69408/#comment295619> Maybe a `LOG.trace()` for better traceability in problematic cases. core/src/main/java/org/apache/oozie/util/BufferDrainer.java Lines 123-125 (patched) <https://reviews.apache.org/r/69408/#comment295657> Nice catch :) core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 49 (patched) <https://reviews.apache.org/r/69408/#comment295620> `EXIT_VALUE_ON_ERROR` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 50-51 (patched) <https://reviews.apache.org/r/69408/#comment295621> `@Rule public ExpectedException expectedException = ExpectedException.none();` would be way more readable. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 55 (patched) <https://reviews.apache.org/r/69408/#comment295622> `<p>` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 58 (patched) <https://reviews.apache.org/r/69408/#comment295623> `<p>` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 63 (patched) <https://reviews.apache.org/r/69408/#comment295624> Can it be a `static class`? I'd also make visibility to package private, and separately test logic of this class. Would rather give a more descriptive name, like `BlockingWritesExitValueProcess`. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 67 (patched) <https://reviews.apache.org/r/69408/#comment295628> Maybe better to extract to upper class level, as a `static class`, and test its functionality separately? And name it e.g. `BlockingInputStream`. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 77-82 (patched) <https://reviews.apache.org/r/69408/#comment295627> Would extract to a method and do it on the constructor end. Also unsure whether we should not write in the constructor but at the beginning at each `read()` call. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 91 (patched) <https://reviews.apache.org/r/69408/#comment295649> Would extract to `final boolean someBytesRead` as per [Javadoc](https://docs.oracle.com/javase/7/docs/api/java/io/ByteArrayInputStream.html#read(byte[],%20int,%20int)) core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 102-111 (patched) <https://reviews.apache.org/r/69408/#comment295651> `checkBlockedAndTryWriteNextChunk()` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 117 (patched) <https://reviews.apache.org/r/69408/#comment295626> `tryWriteNextChunk()` might be a better name. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 179-181 (patched) <https://reviews.apache.org/r/69408/#comment295629> Extract method `generateSamples()`. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 194 (patched) <https://reviews.apache.org/r/69408/#comment295641> `testReadSinglePass()` would be a better name. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 195-196 (patched) <https://reviews.apache.org/r/69408/#comment295633> This code snippet appears multiple times. What if it's extracted to another method, and a `Callable` / `Runnable` is given with varying content each time? core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 202 (patched) <https://reviews.apache.org/r/69408/#comment295640> `readSinglePassAndAssert()` would be a better name. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 205 (patched) <https://reviews.apache.org/r/69408/#comment295638> Extracting `false` would enhance readability. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 206 (patched) <https://reviews.apache.org/r/69408/#comment295630> Assertion message should tell about the error state like: `"some characters should have been read but none was"`. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 207 (patched) <https://reviews.apache.org/r/69408/#comment295631> `"read character count mismatch"` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 208 (patched) <https://reviews.apache.org/r/69408/#comment295632> `"content read mismatch"` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 212 (patched) <https://reviews.apache.org/r/69408/#comment295642> `testReadTillAvailable()` would be a better name. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 213-214 (patched) <https://reviews.apache.org/r/69408/#comment295634> This code snippet appears multiple times. What if it's extracted to another method, and a `Callable` / `Runnable` is given with varying content each time? core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 220 (patched) <https://reviews.apache.org/r/69408/#comment295643> `readTillAvailableAndAssert()` would be a better name. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 223 (patched) <https://reviews.apache.org/r/69408/#comment295639> Extracting `true` would enhance readability. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 230-231 (patched) <https://reviews.apache.org/r/69408/#comment295635> This code snippet appears multiple times. What if it's extracted to another method, and a `Callable` / `Runnable` is given with varying content each time? core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 241-242 (patched) <https://reviews.apache.org/r/69408/#comment295636> This code snippet appears multiple times. What if it's extracted to another method, and a `Callable` / `Runnable` is given with varying content each time? core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 265-266 (patched) <https://reviews.apache.org/r/69408/#comment295637> This code snippet appears multiple times. What if it's extracted to another method, and a `Callable` / `Runnable` is given with varying content each time? core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 277 (patched) <https://reviews.apache.org/r/69408/#comment295654> `before` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 284-289 (patched) <https://reviews.apache.org/r/69408/#comment295655> `expectedException.expect(...)` would be better. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 293 (patched) <https://reviews.apache.org/r/69408/#comment295653> elegant :) core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 296-301 (patched) <https://reviews.apache.org/r/69408/#comment295656> `expectedException.expect(...)` would be better. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 324-325 (patched) <https://reviews.apache.org/r/69408/#comment295645> Not needed, the bespoke refactoring of `BufferDrainer` would help here as well. core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 326 (patched) <https://reviews.apache.org/r/69408/#comment295646> `exitValue` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 330 (patched) <https://reviews.apache.org/r/69408/#comment295647> `String.startsWith()` core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java Lines 332 (patched) <https://reviews.apache.org/r/69408/#comment295648> `String.startsWith()` - András Piros On Nov. 23, 2018, 2:28 p.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69408/ > ----------------------------------------------------------- > > (Updated Nov. 23, 2018, 2:28 p.m.) > > > Review request for oozie, András Piros and Kinga Marton. > > > Repository: oozie-git > > > Description > ------- > > OOZIE-3382 - Optimize SshActionExecutor's drainBuffers method > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/action/ssh/SshActionExecutor.java > 3e0e3c573 > core/src/main/java/org/apache/oozie/util/BufferDrainer.java PRE-CREATION > core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java > PRE-CREATION > > > Diff: https://reviews.apache.org/r/69408/diff/3/ > > > Testing > ------- > > Unit tests executed 50 times using grind. > > > Thanks, > > Andras Salamon > >
