----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/69668/#review211723 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/SqoopActionExecutor.java Lines 258-273 (patched) <https://reviews.apache.org/r/69668/#comment297286> For better readability I would split this in smaller well named methods like: checkAndHandleEscapig(), checkAndHandleQuoting() core/src/test/java/org/apache/oozie/action/hadoop/TestShellSplitter.java Lines 68 (patched) <https://reviews.apache.org/r/69668/#comment297283> Let't try to get some more real life examples as well, and if it is the case expand the tests. Also I would add some edge cases: starting/ending with whitespace, mixed quotation marks, unclosed quotation, trailing escape characters, etc. sharelib/sqoop/src/test/java/org/apache/oozie/action/hadoop/TestSqoopActionExecutor.java Lines 242-244 (patched) <https://reviews.apache.org/r/69668/#comment297278> Please remove commented code - Kinga Marton On Jan. 4, 2019, 3:48 p.m., Andras Salamon wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/69668/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2019, 3:48 p.m.) > > > Review request for oozie, András Piros, Denes Bodo, Peter Cseh, and Kinga > Marton. > > > Repository: oozie-git > > > Description > ------- > > OOZIE 2949 - Escape quotes whitespaces in Sqoop <command> field > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/action/hadoop/SqoopActionExecutor.java > 556f2cfd1 > core/src/test/java/org/apache/oozie/action/hadoop/TestShellSplitter.java > PRE-CREATION > > sharelib/sqoop/src/test/java/org/apache/oozie/action/hadoop/TestSqoopActionExecutor.java > edfe0c739 > > > Diff: https://reviews.apache.org/r/69668/diff/1/ > > > Testing > ------- > > Unit tests locally > > > Thanks, > > Andras Salamon > >
