> On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala, > > line 271 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345581#file1345581line271> > > > > Shouldn't the second be YarnConfig.YARN_KEBEROS_KEYTAB?
Good catch. Thanks. > On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala, > > line 280 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345581#file1345581line280> > > > > Question: this line is writing to HDFS file system. Does that mean that > > the JobRunner that is submitting the application should have Kerberos > > delegation token set up for all HDFS file system access already? Yes. The JobRunner runs at the client side by the user who should have been authenticated already when submitting the application. > On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala, > > line 316 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345581#file1345581line316> > > > > nit: use defined constant string instead of hard-code "credentials" > > here. done > On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala, > > line 89 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345581#file1345581line89> > > > > Since there is need to validate/check non YarnConfig (e.g. JobConfig), > > we should just pass in Config object, to avoid the confusion. Changed to pass Config instead of YarnConfig. > On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala, > > line 179 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345581#file1345581line179> > > > > This is a bit confusing. yarnConfig should only contain YARN specific > > configurations, while coordinator stream config should be in system config. Pass config instead of yarnConfig object. > On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterSecurityManager.scala, > > line 38 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345583#file1345583line38> > > > > nit: Since we are only using thread pool size = 1, it would be clearer > > if we use Executors.newSingleThreadScheduledExecutor() done. > On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala, > > line 112 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345582#file1345582line112> > > > > Question: what if the AM failed and restarted by RM? Wouldn't it be > > missing the keytab files, since it is deleted here? Good point. I actually was never able to test that path (the finally block in the main method), either by trying killing the app via *yarn application -kill command* or sending SIGTERM to the application master jvm. To avoid confusion, I am going to remove this from AM. > On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote: > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala, > > line 173 > > <https://reviews.apache.org/r/37026/diff/4/?file=1345582#file1345582line173> > > > > This is a bit confusing to me: > > 1) I assume that here we are cleaning up the staging resource fils on > > HDFS? If that's the case, how does RM restarts a failed AM (w/o user > > re-submitting the job)? I thought that here we should only clean up local > > resources on AM host? > > 2) It seems that we can reuse the code in ClientHelper since they are > > almost identical Same as above. Remove the deletion of staging directory from the AM main method. The ideal way to handle the clean up of staging directory would be. * The client side, if we make JobRunner keep polling status of application after submission and clean up staging dir for example. * Cleanup can also be managed separately by users post job completion. - Chen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37026/#review130672 ----------------------------------------------------------- On April 14, 2016, 9:26 p.m., Chen Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37026/ > ----------------------------------------------------------- > > (Updated April 14, 2016, 9:26 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > Basic support for kerberization based on keytab authentication. > > > Diffs > ----- > > build.gradle 16facbb > samza-core/src/main/java/org/apache/samza/container/SecurityManager.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/SecurityManagerFactory.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala 4d5ca4d > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 5462208 > samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java c556d83 > samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala > 74a0676 > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala > 80deb3b > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterSecurityManager.scala > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterService.scala > 3adf86f > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaContainerSecurityManager.scala > PRE-CREATION > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaYarnSecurityManagerFactory.scala > PRE-CREATION > samza-yarn/src/main/scala/org/apache/samza/job/yarn/YarnJob.scala 62ddb26 > samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestClientHelper.scala > PRE-CREATION > > samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterService.scala > 7f5d9f4 > > Diff: https://reviews.apache.org/r/37026/diff/ > > > Testing > ------- > > > Thanks, > > Chen Song > >
