Re: Review Request 41106: SAMZA-833: ProcessJob mishandling containers

[Tao Feng]

---
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

[Jake Maes]

---
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)


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

[Boris Shkolnik]

---
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

[Tao Feng]

> 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
> > 
> >
> > 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

[Tao Feng]

---
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

[Jake Maes]

---
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

[Yi Pan (Data Infrastructure)]

---
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)


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

[Tao Feng]

---
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