You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chen Song <ch...@hotmail.com> on 2015/08/02 21:49:22 UTC
Review Request 37026: SAMZA-727: Support for Kerberos
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
Use resource manager's security token as the renewer.
Diffs
-----
samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala a2b92798a78d2f489e8a8fc082b2bb44a734a6cc
Diff: https://reviews.apache.org/r/37026/diff/
Testing
-------
Thanks,
Chen Song
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review136742
-----------------------------------------------------------
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaYarnSecurityManagerFactory.scala (line 1)
<https://reviews.apache.org/r/37026/#comment201842>
rat error. Missing license header.
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestClientHelper.scala (line 1)
<https://reviews.apache.org/r/37026/#comment201841>
rat error. Need license header as well.
- Yi Pan (Data Infrastructure)
On June 8, 2016, 7:10 p.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated June 8, 2016, 7:10 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 951d479
> 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 7bd8131
> 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/main/scala/org/apache/samza/job/yarn/YarnJobUtil.scala PRE-CREATION
> 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 fc0091f
>
> Diff: https://reviews.apache.org/r/37026/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chen Song
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review136733
-----------------------------------------------------------
Fix it, then Ship it!
LGTM. We are also trying to test it internally in LinkedIn as well. If you can update w/ the fixes soon, we can verify the patch in parallel and merge it quickly. Thanks a lot!
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala (line 114)
<https://reviews.apache.org/r/37026/#comment201831>
Just confirmed w/ Jagadish who had observed the application's behavior with final application status before, we actually need to cleanup even in FinalApplicationStatus.FAILED case as well. The only case that we should leave it is the "UNDEFINED" state. Any defined FinalApplicationStatus means that YARN won't try to restart the AM. Hence, we should cleanup. I have included that logic in patch-7 earlier (https://issues.apache.org/jira/secure/attachment/12808018/SAMZA-727.7.patch).
samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java (line 110)
<https://reviews.apache.org/r/37026/#comment201832>
nit: prefer to fix the typo and make the constant string to be YARN_KERBEROS_PRINCIPAL and YARN_KERBEROS_KEYTAB
samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java (line 197)
<https://reviews.apache.org/r/37026/#comment201833>
Same here. Should be getYarnKerberosPrincipal() and getYarnKerberosKeytab()
- Yi Pan (Data Infrastructure)
On June 8, 2016, 7:10 p.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated June 8, 2016, 7:10 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 951d479
> 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 7bd8131
> 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/main/scala/org/apache/samza/job/yarn/YarnJobUtil.scala PRE-CREATION
> 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 fc0091f
>
> Diff: https://reviews.apache.org/r/37026/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chen Song
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review136744
-----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/container/SecurityManager.java (line 26)
<https://reviews.apache.org/r/37026/#comment201844>
checkstyle error: indent should be consistent w/ 2-spaces
samza-core/src/main/java/org/apache/samza/container/SecurityManagerFactory.java (line 28)
<https://reviews.apache.org/r/37026/#comment201845>
Same checkstyle error: 2-space indent
- Yi Pan (Data Infrastructure)
On June 8, 2016, 7:10 p.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated June 8, 2016, 7:10 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 951d479
> 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 7bd8131
> 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/main/scala/org/apache/samza/job/yarn/YarnJobUtil.scala PRE-CREATION
> 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 fc0091f
>
> Diff: https://reviews.apache.org/r/37026/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Chen Song
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/
-----------------------------------------------------------
(Updated June 8, 2016, 7:10 p.m.)
Review request for samza.
Repository: samza
Description
-------
Basic support for kerberization based on keytab authentication.
Diffs (updated)
-----
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 951d479
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 7bd8131
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/main/scala/org/apache/samza/job/yarn/YarnJobUtil.scala PRE-CREATION
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 fc0091f
Diff: https://reviews.apache.org/r/37026/diff/
Testing
-------
Thanks,
Chen Song
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
> On May 24, 2016, 3:31 a.m., Yi Pan (Data Infrastructure) wrote:
> > Hi, Chen, thanks for the update! One more comment: please rebase the patch against the latest master s.t. it can be applied cleanly.
> >
> > Thanks!
>
> Yi Pan (Data Infrastructure) wrote:
> Before it is too late. I tried to rebase and ran the compilation and notice that there are some :rat errors on the following two files:
> Unknown license: /home/yipan/workspace/samza_master/samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaYarnSecurityManagerFactory.scala
> Unknown license: /home/yipan/workspace/samza_master/samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestClientHelper.scala
>
>
> Please add the Apache license headers to those files.
Please run gradlew clean check as well. I saw the following checkstyle errors:
:samza-core_2.10:checkstyleMain
[ant:checkstyle] /home/yipan/workspace/samza_master/samza-core/src/main/java/org/apache/samza/container/SecurityManager.java:26: method def return type at indentation level 4 not at correct indentation, 2
[ant:checkstyle] /home/yipan/workspace/samza_master/samza-core/src/main/java/org/apache/samza/container/SecurityManager.java:28: method def return type at indentation level 4 not at correct indentation, 2
[ant:checkstyle] /home/yipan/workspace/samza_master/samza-core/src/main/java/org/apache/samza/container/SecurityManagerFactory.java:28: method def return type at indentation level 4 not at correct indentation, 2
- Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review134501
-----------------------------------------------------------
On May 8, 2016, 12:50 a.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 12:50 a.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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
> On May 24, 2016, 3:31 a.m., Yi Pan (Data Infrastructure) wrote:
> > Hi, Chen, thanks for the update! One more comment: please rebase the patch against the latest master s.t. it can be applied cleanly.
> >
> > Thanks!
Before it is too late. I tried to rebase and ran the compilation and notice that there are some :rat errors on the following two files:
Unknown license: /home/yipan/workspace/samza_master/samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaYarnSecurityManagerFactory.scala
Unknown license: /home/yipan/workspace/samza_master/samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestClientHelper.scala
Please add the Apache license headers to those files.
- Yi
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review134501
-----------------------------------------------------------
On May 8, 2016, 12:50 a.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 12:50 a.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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
> On May 24, 2016, 3:31 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala, line 111
> > <https://reviews.apache.org/r/37026/diff/4-6/?file=1345582#file1345582line111>
> >
> > Instead of removing this completely, I think it would be better to keep this code when the application exiting status is either "SUCCEEDED" or "KILLED" (defined in FinalApplicationStatus class).
Good point. I will refactor.
- Chen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review134501
-----------------------------------------------------------
On May 8, 2016, 12:50 a.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 12:50 a.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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review134501
-----------------------------------------------------------
Fix it, then Ship it!
Hi, Chen, thanks for the update! One more comment: please rebase the patch against the latest master s.t. it can be applied cleanly.
Thanks!
samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java (line 110)
<https://reviews.apache.org/r/37026/#comment199364>
nit: there are a mis-spelling of "keberos" in this string and the next one, which should be corrected:
- YARN_KEBEROS_PRINCIPAL ==> YARN_KERBEROS_PRINCIPAL
- YARN_KEBEROS_KEYTAB ==> YARN_KERBEROS_KEYTAB
samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java (line 197)
<https://reviews.apache.org/r/37026/#comment199365>
Mis-spelling here as well: "keberos" should be "kerberos".
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala
<https://reviews.apache.org/r/37026/#comment199370>
Instead of removing this completely, I think it would be better to keep this code when the application exiting status is either "SUCCEEDED" or "KILLED" (defined in FinalApplicationStatus class).
- Yi Pan (Data Infrastructure)
On May 8, 2016, 12:50 a.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 12:50 a.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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review134608
-----------------------------------------------------------
I was able to build after fixing the test class methods. Please fix this and run ./bin/check-all.sh to make sure that it passes all tests. Thanks!
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestClientHelper.scala (line 42)
<https://reviews.apache.org/r/37026/#comment199451>
This should be changed to getSecurityYarnConfig as well.
- Yi Pan (Data Infrastructure)
On May 8, 2016, 12:50 a.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 12:50 a.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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/
-----------------------------------------------------------
(Updated May 8, 2016, 12:50 a.m.)
Review request for samza.
Changes
-------
fix typo
Repository: samza
Description
-------
Basic support for kerberization based on keytab authentication.
Diffs (updated)
-----
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
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/
-----------------------------------------------------------
(Updated May 7, 2016, 10:38 p.m.)
Review request for samza.
Changes
-------
Chaanges based on Yi Pan's comments.
Repository: samza
Description
-------
Basic support for kerberization based on keytab authentication.
Diffs (updated)
-----
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
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
> On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java, line 131
> > <https://reviews.apache.org/r/37026/diff/4/?file=1345580#file1345580line131>
> >
> > This configuration is missing in the doc patch.
Will update the doc patch.
> 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 37
> > <https://reviews.apache.org/r/37026/diff/4/?file=1345583#file1345583line37>
> >
> > It would be nice to add some javadoc 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/YarnJob.scala, line 63
> > <https://reviews.apache.org/r/37026/diff/4/?file=1345587#file1345587line63>
> >
> > nit: since submitApplication would require to access non yarnConfig as well, change it to config.
Config will be used to be passed to submitApplication.
- Chen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review130672
-----------------------------------------------------------
On May 8, 2016, 12:50 a.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 12:50 a.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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
> On April 27, 2016, 9:16 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java, line 131
> > <https://reviews.apache.org/r/37026/diff/4/?file=1345580#file1345580line131>
> >
> > This configuration is missing in the doc patch.
>
> Chen Song wrote:
> Will update the doc patch.
Sorry this config is auto-generated by ClientHelper and sent to the coordinator stream. It is not set explicitly as a configuration by user. I will elaborate it a bit more in the yarn-security.md doc page. However, I am not sure whether it needs to be reflected in the configuration html page.
Let me know your thoughts on this.
- Chen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/#review130672
-----------------------------------------------------------
On May 8, 2016, 12:50 a.m., Chen Song wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37026/
> -----------------------------------------------------------
>
> (Updated May 8, 2016, 12:50 a.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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
> 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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
-----------------------------------------------------------
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 (updated)
-------
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
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
-----------------------------------------------------------
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
-------
Use resource manager's security token as the renewer.
Diffs (updated)
-----
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
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/
-----------------------------------------------------------
(Updated April 14, 2016, 9:22 p.m.)
Review request for samza.
Repository: samza
Description
-------
Use resource manager's security token as the renewer.
Diffs (updated)
-----
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
Re: Review Request 37026: SAMZA-727: Support for Kerberos
Posted by Chen Song <ch...@hotmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/37026/
-----------------------------------------------------------
(Updated Dec. 3, 2015, 9:05 p.m.)
Review request for samza.
Repository: samza
Description
-------
Use resource manager's security token as the renewer.
Diffs (updated)
-----
samza-yarn/src/main/scala/org/apache/samza/job/yarn/ClientHelper.scala 74a0676
Diff: https://reviews.apache.org/r/37026/diff/
Testing
-------
Thanks,
Chen Song