You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Andras Salamon <an...@melda.info> on 2018/11/20 11:38:44 UTC
Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's
drainBuffers method
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69408/
-----------------------------------------------------------
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/1/
Testing
-------
Unit tests
Thanks,
Andras Salamon
Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's
drainBuffers method
Posted by Andras Salamon <an...@melda.info>.
> On Nov. 26, 2018, 12:57 p.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
> > Lines 49 (patched)
> > <https://reviews.apache.org/r/69408/diff/3/?file=2110238#file2110238line49>
> >
> > `EXIT_VALUE_ON_ERROR`
The exit value is always 1 not only on error but on the successful finish as well.
> On Nov. 26, 2018, 12:57 p.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java
> > Lines 50-51 (patched)
> > <https://reviews.apache.org/r/69408/diff/3/?file=2110238#file2110238line50>
> >
> > `@Rule public ExpectedException expectedException = ExpectedException.none();` would be way more readable.
This is not really an expected exception. Junit only catches the exception in the main thread. My solution is based on this suggestion: https://stackoverflow.com/a/2596530/21348
- Andras
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69408/#review210855
-----------------------------------------------------------
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
>
>
Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's
drainBuffers method
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's
drainBuffers method
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69408/#review210997
-----------------------------------------------------------
Ship it!
Ship It!
- András Piros
On Nov. 30, 2018, 4:04 p.m., Andras Salamon wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69408/
> -----------------------------------------------------------
>
> (Updated Nov. 30, 2018, 4:04 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/BlockingInputStream.java PRE-CREATION
> core/src/test/java/org/apache/oozie/util/BlockingWritesExitValueProcess.java PRE-CREATION
> core/src/test/java/org/apache/oozie/util/DrainerTestCase.java PRE-CREATION
> core/src/test/java/org/apache/oozie/util/TestBlockingInputStream.java PRE-CREATION
> core/src/test/java/org/apache/oozie/util/TestBlockingWritesExitValueProcess.java PRE-CREATION
> core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/69408/diff/4/
>
>
> Testing
> -------
>
> Unit tests executed 50 times using grind.
>
>
> Thanks,
>
> Andras Salamon
>
>
Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's
drainBuffers method
Posted by Andras Salamon <an...@melda.info>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69408/
-----------------------------------------------------------
(Updated Nov. 30, 2018, 4:04 p.m.)
Review request for oozie, András Piros and Kinga Marton.
Changes
-------
Addressing review changes. Lots of refactor.
Repository: oozie-git
Description
-------
OOZIE-3382 - Optimize SshActionExecutor's drainBuffers method
Diffs (updated)
-----
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/BlockingInputStream.java PRE-CREATION
core/src/test/java/org/apache/oozie/util/BlockingWritesExitValueProcess.java PRE-CREATION
core/src/test/java/org/apache/oozie/util/DrainerTestCase.java PRE-CREATION
core/src/test/java/org/apache/oozie/util/TestBlockingInputStream.java PRE-CREATION
core/src/test/java/org/apache/oozie/util/TestBlockingWritesExitValueProcess.java PRE-CREATION
core/src/test/java/org/apache/oozie/util/TestBufferDrainer.java PRE-CREATION
Diff: https://reviews.apache.org/r/69408/diff/4/
Changes: https://reviews.apache.org/r/69408/diff/3-4/
Testing
-------
Unit tests executed 50 times using grind.
Thanks,
Andras Salamon
Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's
drainBuffers method
Posted by Andras Salamon <an...@melda.info>.
-----------------------------------------------------------
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.
Changes
-------
Fixing test flakiness.
Repository: oozie-git
Description
-------
OOZIE-3382 - Optimize SshActionExecutor's drainBuffers method
Diffs (updated)
-----
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/
Changes: https://reviews.apache.org/r/69408/diff/2-3/
Testing (updated)
-------
Unit tests executed 50 times using grind.
Thanks,
Andras Salamon
Re: Review Request 69408: OOZIE-3382 - Optimize SshActionExecutor's
drainBuffers method
Posted by Andras Salamon <an...@melda.info>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69408/
-----------------------------------------------------------
(Updated Nov. 20, 2018, 2:21 p.m.)
Review request for oozie, András Piros and Kinga Marton.
Changes
-------
Fixing RAT warning
Repository: oozie-git
Description
-------
OOZIE-3382 - Optimize SshActionExecutor's drainBuffers method
Diffs (updated)
-----
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/2/
Changes: https://reviews.apache.org/r/69408/diff/1-2/
Testing
-------
Unit tests
Thanks,
Andras Salamon