You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Tao Feng <fe...@gmail.com> on 2015/12/08 23:49:45 UTC
Review Request 41106: SAMZA-833: ProcessJob mishandling containers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/
-----------------------------------------------------------
Review request for samza.
Repository: samza
Description
-------
throws runtime exception if user sets yarn.container.count or job.container.count greater than 1 for ProcessJobFactory
Diffs
-----
samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
Diff: https://reviews.apache.org/r/41106/diff/
Testing
-------
1. ./gradlew clean build
2. ./gradlew checkstyleMain checkstyleTest
3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
Thanks,
Tao Feng
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Tao Feng <fe...@gmail.com>.
> On Dec. 8, 2015, 11:20 p.m., Yi Pan (Data Infrastructure) wrote:
> > samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala, line 54
> > <https://reviews.apache.org/r/41106/diff/1/?file=1156584#file1156584line54>
> >
> > It would be better to use JobConfig.getContainerCount() instead of repeating the code here. The following line does the trick for you to use config object as JobConfig object:
> >
> > import org.apache.samza.config.JobConfig.Config2Job
Update rb per comment
- Tao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/#review109428
-----------------------------------------------------------
On Dec. 9, 2015, 6:41 a.m., Tao Feng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41106/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2015, 6:41 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> 1. throws runtime exception if user sets yarn.container.count or job.container.count larger than 1 for ProcessJobFactory;
> 2. update per Yi's comment
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
>
> Diff: https://reviews.apache.org/r/41106/diff/
>
>
> Testing
> -------
>
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
>
>
> Thanks,
>
> Tao Feng
>
>
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by "Yi Pan (Data Infrastructure)" <yi...@linkedin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/#review109428
-----------------------------------------------------------
Thanks, Tao! One issue in the comment.
samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala (line 54)
<https://reviews.apache.org/r/41106/#comment168948>
It would be better to use JobConfig.getContainerCount() instead of repeating the code here. The following line does the trick for you to use config object as JobConfig object:
import org.apache.samza.config.JobConfig.Config2Job
- Yi Pan (Data Infrastructure)
On Dec. 8, 2015, 10:49 p.m., Tao Feng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41106/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2015, 10:49 p.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> throws runtime exception if user sets yarn.container.count or job.container.count greater than 1 for ProcessJobFactory
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
>
> Diff: https://reviews.apache.org/r/41106/diff/
>
>
> Testing
> -------
>
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
>
>
> Thanks,
>
> Tao Feng
>
>
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Boris Shkolnik <bo...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/#review109619
-----------------------------------------------------------
Ship it!
- Boris Shkolnik
On Dec. 9, 2015, 6:41 a.m., Tao Feng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41106/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2015, 6:41 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> 1. throws runtime exception if user sets yarn.container.count or job.container.count larger than 1 for ProcessJobFactory;
> 2. update per Yi's comment
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
>
> Diff: https://reviews.apache.org/r/41106/diff/
>
>
> Testing
> -------
>
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
>
>
> Thanks,
>
> Tao Feng
>
>
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/#review109687
-----------------------------------------------------------
Ship it!
Ship It!
- Jake Maes
On Dec. 10, 2015, 4:34 a.m., Tao Feng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41106/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 4:34 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> update per Jake's comment
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
>
> Diff: https://reviews.apache.org/r/41106/diff/
>
>
> Testing
> -------
>
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
>
>
> Thanks,
>
> Tao Feng
>
>
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Tao Feng <fe...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/
-----------------------------------------------------------
(Updated April 15, 2016, 3:55 a.m.)
Review request for samza.
Repository: samza
Description (updated)
-------
rebase branch
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 17c2e5be9ee216c88e3c07784c4f9c05cd92e9c9
Diff: https://reviews.apache.org/r/41106/diff/
Testing
-------
1. ./gradlew clean build
2. ./gradlew checkstyleMain checkstyleTest
3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
Thanks,
Tao Feng
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Tao Feng <fe...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/
-----------------------------------------------------------
(Updated Dec. 10, 2015, 4:34 a.m.)
Review request for samza.
Repository: samza
Description (updated)
-------
update per Jake's comment
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
Diff: https://reviews.apache.org/r/41106/diff/
Testing
-------
1. ./gradlew clean build
2. ./gradlew checkstyleMain checkstyleTest
3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
Thanks,
Tao Feng
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Tao Feng <fe...@gmail.com>.
> On Dec. 9, 2015, 6:06 p.m., Jake Maes wrote:
> > samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala, line 38
> > <https://reviews.apache.org/r/41106/diff/2/?file=1156859#file1156859line38>
> >
> > nit: this message will no longer be valid after we fully deprecate yarn.container.count. It would be more future-proof to say "Container count larger than 1..."
Thanks for the suggestion. I will send out an updated RB later today.
- Tao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/#review109571
-----------------------------------------------------------
On Dec. 9, 2015, 6:41 a.m., Tao Feng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41106/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2015, 6:41 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> 1. throws runtime exception if user sets yarn.container.count or job.container.count larger than 1 for ProcessJobFactory;
> 2. update per Yi's comment
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
>
> Diff: https://reviews.apache.org/r/41106/diff/
>
>
> Testing
> -------
>
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
>
>
> Thanks,
>
> Tao Feng
>
>
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Jake Maes <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/#review109571
-----------------------------------------------------------
Ship it!
samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala (line 38)
<https://reviews.apache.org/r/41106/#comment169146>
nit: this message will no longer be valid after we fully deprecate yarn.container.count. It would be more future-proof to say "Container count larger than 1..."
- Jake Maes
On Dec. 9, 2015, 6:41 a.m., Tao Feng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41106/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2015, 6:41 a.m.)
>
>
> Review request for samza.
>
>
> Repository: samza
>
>
> Description
> -------
>
> 1. throws runtime exception if user sets yarn.container.count or job.container.count larger than 1 for ProcessJobFactory;
> 2. update per Yi's comment
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
>
> Diff: https://reviews.apache.org/r/41106/diff/
>
>
> Testing
> -------
>
> 1. ./gradlew clean build
> 2. ./gradlew checkstyleMain checkstyleTest
> 3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
>
>
> Thanks,
>
> Tao Feng
>
>
Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers
Posted by Tao Feng <fe...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41106/
-----------------------------------------------------------
(Updated Dec. 9, 2015, 6:41 a.m.)
Review request for samza.
Repository: samza
Description (updated)
-------
1. throws runtime exception if user sets yarn.container.count or job.container.count larger than 1 for ProcessJobFactory;
2. update per Yi's comment
Diffs (updated)
-----
samza-core/src/main/scala/org/apache/samza/job/local/ProcessJobFactory.scala 0792a59cb7b042220c8dbfc0713c5ef42e93ab25
Diff: https://reviews.apache.org/r/41106/diff/
Testing
-------
1. ./gradlew clean build
2. ./gradlew checkstyleMain checkstyleTest
3. manual test open source hello samza and verify that by giving yarn.container.count or job.container.count with value greater than 1 for ProcessJobFactory, the test will throw the desired exception.
Thanks,
Tao Feng