----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/63327/#review189427 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 154-155 (patched) <https://reviews.apache.org/r/63327/#comment266515> Should go to `LauncherAM#OOZIE_LAUNCHER_MODIFY_ACL`. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 155 (patched) <https://reviews.apache.org/r/63327/#comment266516> Should go to `LauncherAM#OOZIE_LAUNCHER_VIEW_ACL`. core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java Lines 1309-1325 (patched) <https://reviews.apache.org/r/63327/#comment266526> I think we should take `mapreduce.cluster.acls.enabled` / `mapred.acls.enabled` into consideration, shouldn't we? Default is `false`, in that case we shouldn't set ACLs even if the user has employed `oozie.launcher.acl-view-job` / `oozie.launcher.acl-modify-job`. core/src/main/java/org/apache/oozie/workflow/lite/LauncherConfigHandler.java Lines 34-35 (patched) <https://reviews.apache.org/r/63327/#comment266527> Please also extend `oozie-default.xml` with proper override key / value pairs like: * `oozie.launcher.override.acl-view-job` -> `mapreduce.job.acl-view-job` * `oozie.launcher.override.acl-modify-job` -> `mapreduce.job.acl-modify-job` and extend unit test cases regarding that aspect, as well. core/src/main/java/org/apache/oozie/workflow/lite/LauncherConfigHandler.java Lines 69 (patched) <https://reviews.apache.org/r/63327/#comment266517> Use `LauncherAM#OOZIE_LAUNCHER_VIEW_ACL`. core/src/main/java/org/apache/oozie/workflow/lite/LauncherConfigHandler.java Lines 70 (patched) <https://reviews.apache.org/r/63327/#comment266518> Use `LauncherAM#OOZIE_LAUNCHER_MODIFY_ACL`. core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java Lines 88-94 (patched) <https://reviews.apache.org/r/63327/#comment266519> Extract method `checkAndSleep(args)`. core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java Lines 1659 (patched) <https://reviews.apache.org/r/63327/#comment266520> Very good naming! :) core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java Lines 1659-1687 (patched) <https://reviews.apache.org/r/63327/#comment266521> In general, that's a good negative test. What about a positive one - killing succeeds w/ the given user? What about another one when `mapreduce.cluster.acls.enabled` is left `false` (the feature shouldn't work)? core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java Lines 1676-1683 (patched) <https://reviews.apache.org/r/63327/#comment266522> Would extract method `modifyUserAndKill(String userId)`. core/src/test/java/org/apache/oozie/test/XTestCase.java Lines 1031-1032 (patched) <https://reviews.apache.org/r/63327/#comment266524> That's something I'd set per test case. I don't think we should in general override the default value for all the `? extends XTestCase`s. core/src/test/java/org/apache/oozie/test/XTestCase.java Lines 1033-1034 (patched) <https://reviews.apache.org/r/63327/#comment266525> Please extract method / comment why it's needed. - András Piros On Oct. 26, 2017, 12:24 p.m., Peter Bacsko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/63327/ > ----------------------------------------------------------- > > (Updated Oct. 26, 2017, 12:24 p.m.) > > > Review request for oozie, András Piros, Attila Sasvari, Peter Cseh, and > Robert Kanter. > > > Repository: oozie-git > > > Description > ------- > > https://issues.apache.org/jira/browse/OOZIE-2897 > > > Diffs > ----- > > client/src/main/resources/oozie-common-1.0.xsd ddae91224 > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > 76d0daa4f > > core/src/main/java/org/apache/oozie/workflow/lite/LauncherConfigHandler.java > c36774239 > core/src/test/java/org/apache/oozie/action/hadoop/LauncherMainTester.java > 43c71b0ad > > core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java > a7bd357f8 > core/src/test/java/org/apache/oozie/service/TestSchemaService.java > 940868aeb > core/src/test/java/org/apache/oozie/test/XTestCase.java 584aa12ef > > core/src/test/java/org/apache/oozie/workflow/lite/TestLiteWorkflowAppParser.java > a361078ac > core/src/test/resources/fair-scheduler-alloc.xml PRE-CREATION > core/src/test/resources/wf-schema-global-launcherconf.xml 9cd4f6c37 > > > Diff: https://reviews.apache.org/r/63327/diff/1/ > > > Testing > ------- > > - Unit test added > - Verified on a 4-node cluster > > > Thanks, > > Peter Bacsko > >
