You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Jacob Tolar <de...@sheckel.net> on 2017/12/16 00:18:19 UTC
Review Request 64663: [OOZIE-2150] Shell launcher should print shell
script
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64663/
-----------------------------------------------------------
Review request for oozie.
Repository: oozie-git
Description
-------
[OOZIE-2150] Shell launcher should print shell script
Oozie shell launcher will dump script into log. The script is printed if it is (1) not too big and (2) appears to be plain-text (no null bytes).
Diffs
-----
core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java 1a654f72
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java 55d3d965
Diff: https://reviews.apache.org/r/64663/diff/1/
Testing
-------
unit test added
Thanks,
Jacob Tolar
Re: Review Request 64663: [OOZIE-2150] Shell launcher should print
shell script
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/64663/#review194742
-----------------------------------------------------------
While I think your changes point to a very good direction, here are my two more cents.
core/src/test/java/org/apache/oozie/action/hadoop/TestShellContentWriter.java
Lines 140 (patched)
<https://reviews.apache.org/r/64663/#comment273713>
For the negative-or-zero preconfigured value case I'd reformulate the error string as something like:
```Not printing script file as configured, content suppressed.```
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellContentWriter.java
Lines 64 (patched)
<https://reviews.apache.org/r/64663/#comment273714>
For the negative-or-zero preconfigured value case I'd reformulate the error string as something like:
```Not printing script file as configured, content suppressed.```
- András Piros
On Jan. 3, 2018, 7:39 p.m., Jacob Tolar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64663/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2018, 7:39 p.m.)
>
>
> Review request for oozie.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2150] Shell launcher should print shell script
>
> Oozie shell launcher will dump script into log. The script is printed if it is (1) not too big and (2) appears to be plain-text (no null bytes).
>
>
> Diffs
> -----
>
> core/src/main/resources/oozie-default.xml 1c348007
> core/src/test/java/org/apache/oozie/action/hadoop/TestShellContentWriter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellContentWriter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java 55d3d965
>
>
> Diff: https://reviews.apache.org/r/64663/diff/2/
>
>
> Testing
> -------
>
> unit test added
>
>
> Thanks,
>
> Jacob Tolar
>
>
Re: Review Request 64663: [OOZIE-2150] Shell launcher should print
shell script
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/64663/#review194844
-----------------------------------------------------------
Ship it!
Ship It!
- András Piros
On Jan. 4, 2018, 6:38 p.m., Jacob Tolar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64663/
> -----------------------------------------------------------
>
> (Updated Jan. 4, 2018, 6:38 p.m.)
>
>
> Review request for oozie.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2150] Shell launcher should print shell script
>
> Oozie shell launcher will dump script into log. The script is printed if it is (1) not too big and (2) appears to be plain-text (no null bytes).
>
>
> Diffs
> -----
>
> core/src/main/resources/oozie-default.xml 1c348007
> core/src/test/java/org/apache/oozie/action/hadoop/TestShellContentWriter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellContentWriter.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java 55d3d965
>
>
> Diff: https://reviews.apache.org/r/64663/diff/3/
>
>
> Testing
> -------
>
> unit test added
>
>
> Thanks,
>
> Jacob Tolar
>
>
Re: Review Request 64663: [OOZIE-2150] Shell launcher should print
shell script
Posted by Jacob Tolar <de...@sheckel.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64663/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 6:38 p.m.)
Review request for oozie.
Changes
-------
address review comments
add helper method to simplify tests
Repository: oozie-git
Description
-------
[OOZIE-2150] Shell launcher should print shell script
Oozie shell launcher will dump script into log. The script is printed if it is (1) not too big and (2) appears to be plain-text (no null bytes).
Diffs (updated)
-----
core/src/main/resources/oozie-default.xml 1c348007
core/src/test/java/org/apache/oozie/action/hadoop/TestShellContentWriter.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellContentWriter.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java 55d3d965
Diff: https://reviews.apache.org/r/64663/diff/3/
Changes: https://reviews.apache.org/r/64663/diff/2-3/
Testing
-------
unit test added
Thanks,
Jacob Tolar
Re: Review Request 64663: [OOZIE-2150] Shell launcher should print
shell script
Posted by Jacob Tolar <de...@sheckel.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64663/
-----------------------------------------------------------
(Updated Jan. 4, 2018, 6:35 p.m.)
Review request for oozie.
Changes
-------
address review comments
add helper method to simplify tests
Repository: oozie-git
Description
-------
[OOZIE-2150] Shell launcher should print shell script
Oozie shell launcher will dump script into log. The script is printed if it is (1) not too big and (2) appears to be plain-text (no null bytes).
Diffs
-----
core/src/main/resources/oozie-default.xml 1c348007
core/src/test/java/org/apache/oozie/action/hadoop/TestShellContentWriter.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellContentWriter.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java 55d3d965
Diff: https://reviews.apache.org/r/64663/diff/2/
Testing
-------
unit test added
File Attachments (updated)
----------------
OOZIE-2150-003.patch
https://reviews.apache.org/media/uploaded/files/2018/01/04/3c03fb61-5274-4965-b53a-84b1c29b8e3f__OOZIE-2150-003.patch
Thanks,
Jacob Tolar
Re: Review Request 64663: [OOZIE-2150] Shell launcher should print
shell script
Posted by Jacob Tolar <de...@sheckel.net>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64663/
-----------------------------------------------------------
(Updated Jan. 3, 2018, 7:39 p.m.)
Review request for oozie.
Changes
-------
update to address review concerns
* create ShellContentWriter class and test class
* update temp file creation to use Junit TemporaryFolder
* Make max size configurable
* other review suggestions
Repository: oozie-git
Description
-------
[OOZIE-2150] Shell launcher should print shell script
Oozie shell launcher will dump script into log. The script is printed if it is (1) not too big and (2) appears to be plain-text (no null bytes).
Diffs (updated)
-----
core/src/main/resources/oozie-default.xml 1c348007
core/src/test/java/org/apache/oozie/action/hadoop/TestShellContentWriter.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellContentWriter.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java 55d3d965
Diff: https://reviews.apache.org/r/64663/diff/2/
Changes: https://reviews.apache.org/r/64663/diff/1-2/
Testing
-------
unit test added
Thanks,
Jacob Tolar
Re: Review Request 64663: [OOZIE-2150] Shell launcher should print
shell script
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/64663/#review194046
-----------------------------------------------------------
core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java
Lines 67-81 (patched)
<https://reviews.apache.org/r/64663/#comment272758>
Let's take into separate test class `TestShellContentWriter`, and have different and well-named test cases for each scenario.
Apart from this, what about more elaborate test cases like:
* too long scripts
* scripts with 0 bytes
* files containing not only `0x00` but `0x09` as well
core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java
Lines 68-69 (patched)
<https://reviews.apache.org/r/64663/#comment272759>
What about using JUnit 4's `TemporaryFolder`?
core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java
Lines 84-94 (patched)
<https://reviews.apache.org/r/64663/#comment272757>
If you have `ShellContentWriter` and inject an `OutputStream` via constructor, you'll be able to test it with a mock `OutputStream` without the need of setting and resetting `System.out` or `System.err`.
core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java
Lines 86 (patched)
<https://reviews.apache.org/r/64663/#comment272756>
Use preconfigured value here.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Lines 55 (patched)
<https://reviews.apache.org/r/64663/#comment272747>
Let's call this constant something like `MAX_SCRIPT_SIZE_TO_PRINT_KB`, and make it configurable via `ConfigurationService`. Have a sane default value within `oozie-default.xml`, too.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Lines 331 (patched)
<https://reviews.apache.org/r/64663/#comment272749>
Some sanity checks on `cmdArray` wouldn't harm.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Lines 333-334 (original), 348-386 (patched)
<https://reviews.apache.org/r/64663/#comment272750>
Extract to - at least nested - class `ShellContentWriter` that takes `OutputStream` and `fileName` as parameters.
You can then conveniently unit test that.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Lines 357 (patched)
<https://reviews.apache.org/r/64663/#comment272752>
What about files with 0 bytes?
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Lines 358 (patched)
<https://reviews.apache.org/r/64663/#comment272751>
I wouldn't read all the bytes, but read 1-by-1, and check whether the actual one is under `0x09`. If so, it's handled as a binary file, else as a shell script.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Lines 370 (patched)
<https://reviews.apache.org/r/64663/#comment272753>
Maybe `System.err.println()` the error messages.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Lines 373-375 (patched)
<https://reviews.apache.org/r/64663/#comment272754>
`catch (final IOException ignored) {}` and then you don't need to comment anything.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java
Line 334 (original), 379-386 (patched)
<https://reviews.apache.org/r/64663/#comment272755>
I wouldn't read all the bytes, but read 1-by-1, and check whether the actual one is under `0x09`. If so, it's handled as a binary file, else as a shell script.
Let's not make this one static, and part of `ShellContentWriter`.
- András Piros
On Dec. 16, 2017, 12:18 a.m., Jacob Tolar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64663/
> -----------------------------------------------------------
>
> (Updated Dec. 16, 2017, 12:18 a.m.)
>
>
> Review request for oozie.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> [OOZIE-2150] Shell launcher should print shell script
>
> Oozie shell launcher will dump script into log. The script is printed if it is (1) not too big and (2) appears to be plain-text (no null bytes).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/ShellTestCase.java 1a654f72
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java 55d3d965
>
>
> Diff: https://reviews.apache.org/r/64663/diff/1/
>
>
> Testing
> -------
>
> unit test added
>
>
> Thanks,
>
> Jacob Tolar
>
>