----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61560/#review184556 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java Lines 555 (patched) <https://reviews.apache.org/r/61560/#comment260724> Do we always want to get an MR token? Certainly not for Spark or Shell, but Java, Hive (Hive-on-Spark), and Pig (Pig-on-Spark), are more complicated. I guess it's simpler to just always add the MR token. core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java Lines 559 (patched) <https://reviews.apache.org/r/61560/#comment260725> Add an RM_DELEGATION_TOKEN... core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java Lines 584 (patched) <https://reviews.apache.org/r/61560/#comment260726> Add an MR_DELEGATION_TOKEN... core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java Lines 605 (patched) <https://reviews.apache.org/r/61560/#comment260727> It's good to say this was copied from YARNRunner, but maybe that should be the second sentence. The first sentence should say what this does. "Gets an MR_DELEEGATION_TOKEN from the Job History Server. Copied from YARNRunner in Hadoop." core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java Lines 616 (patched) <https://reviews.apache.org/r/61560/#comment260728> Why do we call this to get the renewer instead of using the same code that the other tokens are using? core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java Lines 627 (patched) <https://reviews.apache.org/r/61560/#comment260729> Same as with the javadoc for getDelegationTokenFromHS - we should have a more descriptive first sentence. core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java Lines 653-654 (patched) <https://reviews.apache.org/r/61560/#comment260730> While this is true (though HDFS HA vs not HA doesn't matter), and that was a tricky issue, I'm not sure this is the right thing to say here. The main reason to get an HDFS token is so that the action can use HDFS, right? Maybe something like: "Add a HDFS_DELEGATION_TOKEN to the @{link Credentials} provided. This is also important to ensure that log aggregation works correctly from the NM" sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java Lines 25 (patched) <https://reviews.apache.org/r/61560/#comment260731> This change is unrelated. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java Line 120 (original), 125 (patched) <https://reviews.apache.org/r/61560/#comment260732> I would add a comment mentioning something about the whole AM being executed inside of a doAs. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java Lines 196 (patched) <https://reviews.apache.org/r/61560/#comment260733> Aren't we already in a doAs? It looks like this is so we can remove the AMRM token. I wonder if instead of doing a nested doAs here, we could instead simply remove the AMRM token from the current UGI? If not, then it's fine to leave this as-is. sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java Line 354 (original), 381 (patched) <https://reviews.apache.org/r/61560/#comment260734> Looks like we can delete this comment - Robert Kanter On Sept. 5, 2017, 1:56 p.m., Peter Cseh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61560/ > ----------------------------------------------------------- > > (Updated Sept. 5, 2017, 1:56 p.m.) > > > Review request for oozie, Attila Sasvari, Peter Bacsko, Robert Kanter, and > Rohini Palaniswamy. > > > Bugs: OOZIE-2909 > https://issues.apache.org/jira/browse/OOZIE-2909 > > > Repository: oozie-git > > > Description > ------- > > Changing UGI calls and cleaning up things around requesting tokens. > > > Diffs > ----- > > core/src/main/java/org/apache/oozie/ErrorCode.java > 662e1edc9c4b23b3606c751bf5ed4b531ee7ac62 > > core/src/main/java/org/apache/oozie/action/hadoop/CredentialsProviderFactory.java > 5ca8d3e80942840a5bba51a06694350929044042 > core/src/main/java/org/apache/oozie/action/hadoop/HCatCredentialHelper.java > 9804c7b673a899cd9c778850c9a64731cc642fb3 > core/src/main/java/org/apache/oozie/action/hadoop/HbaseCredentials.java > 4add5f14cb20ab77c755b81eff939335c0c5fddc > core/src/main/java/org/apache/oozie/action/hadoop/Hive2Credentials.java > 0b495f75842b0033a3337ef6728a334bf5651770 > core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java > bca79aa052521ea4f6f16e76bd69f84fb16be790 > core/src/main/java/org/apache/oozie/service/HadoopAccessorService.java > 187cee2290ba4bbe358913d28a052d9c75f2369a > core/src/test/java/org/apache/oozie/service/TestHadoopAccessorService.java > 960c2f9750062c98fdf7a2b456d3668069e07ca5 > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java > e6c9d04653be984bdd5aa77bee5f28b187e3de5d > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java > b4cbb4b1d9b52aaebfb562c02edc91e15a2a9a2e > > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsOperations.java > 874d371664ced3ae19a7b5d615c67d4c72e7cfdd > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java > 6a98d6ef3a09a7a75272f3f0f9a9c2fc5472e76d > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ShellMain.java > bde7f1daf123d663d47f5d2fb2350cfa1ac33ed8 > > sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestHdfsOperations.java > 68c0f4bafe0322ff0d1007297cd6e8f38c92020a > > sharelib/oozie/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java > 37af3dd9ed43f4504ff3909484237e2d05665c6c > > > Diff: https://reviews.apache.org/r/61560/diff/2/ > > > Testing > ------- > > We're running tests on a Kerberized cluster with these changes. > > > Thanks, > > Peter Cseh > >
