----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37026/#review130672 -----------------------------------------------------------
Thanks, @Chen Song for pulling this off! Overall lgtm. I have a high-level comment regarding to some clarification on how HDFS token used by JobRunner to create the staging directory: I assume that the Kerberos token/credential is initialized for JobRunner via yarnConfig? It would be good to put some comment in the code to clarify this. samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 593) <https://reviews.apache.org/r/37026/#comment194507> Any reason that we want to delay the start up of security manager? It might be useful in starting a HDFS producer as well. samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala (line 760) <https://reviews.apache.org/r/37026/#comment194508> nit: unnecesary empty line here. samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java (line 110) <https://reviews.apache.org/r/37026/#comment194564> nit: YARN_KERBEROS_PRINCIPLE and YARN_KERBEROS_KEYTAB samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java (line 131) <https://reviews.apache.org/r/37026/#comment194509> This configuration is missing in the doc patch. samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala (line 88) <https://reviews.apache.org/r/37026/#comment194541> Since there is need to validate/check non YarnConfig (e.g. JobConfig), we should just pass in Config object, to avoid the confusion. samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala (line 168) <https://reviews.apache.org/r/37026/#comment194526> This is a bit confusing. yarnConfig should only contain YARN specific configurations, while coordinator stream config should be in system config. samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala (line 260) <https://reviews.apache.org/r/37026/#comment194551> Shouldn't the second be YarnConfig.YARN_KEBEROS_KEYTAB? samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala (line 269) <https://reviews.apache.org/r/37026/#comment194563> 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? samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala (line 303) <https://reviews.apache.org/r/37026/#comment194567> Isn't it better to name it as getSecurityYarnConfig()? samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala (line 305) <https://reviews.apache.org/r/37026/#comment194566> nit: use defined constant string instead of hard-code "credentials" here. samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala (line 112) <https://reviews.apache.org/r/37026/#comment194568> Question: what if the AM failed and restarted by RM? Wouldn't it be missing the keytab files, since it is deleted here? samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala (line 173) <https://reviews.apache.org/r/37026/#comment194569> 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 samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterSecurityManager.scala (line 37) <https://reviews.apache.org/r/37026/#comment194570> It would be nice to add some javadoc here. samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterSecurityManager.scala (line 38) <https://reviews.apache.org/r/37026/#comment194572> nit: Since we are only using thread pool size = 1, it would be clearer if we use Executors.newSingleThreadScheduledExecutor() samza-yarn/src/main/scala/org/apache/samza/job/yarn/YarnJob.scala (line 63) <https://reviews.apache.org/r/37026/#comment194739> nit: since submitApplication would require to access non yarnConfig as well, change it to config. - Yi Pan (Data Infrastructure) 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 > >
