You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Chinmay Soman <ch...@gmail.com> on 2014/07/28 23:48:27 UTC
Review Request 24011: SAMZA-109: Make task.opts easier to use
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24011/
-----------------------------------------------------------
Review request for samza.
Bugs: SAMZA-109
https://issues.apache.org/jira/browse/SAMZA-109
Repository: samza
Description
-------
SAMZA-109: Make task.opts easier to use
Diffs
-----
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala e20e7c1bcbc278bd61cf9446090b09d00744abb8
samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala PRE-CREATION
samza-shell/src/main/bash/run-am.sh 878d938329ee0956b8261aee35f73a31111a5223
samza-shell/src/main/bash/run-class.sh a75cbb915659dcf69febe5bc91999225aa21971f
samza-shell/src/main/bash/run-container.sh b43bc3f7e6d7d3df7b3edd8e910f5ac9de548295
samza-test/src/main/resources/hello-stateful-world.samsa 8c2f2e47f77549a772d7d8d7cf922f7bfd1112b6
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala dc44a992083df6f70177aabfa8c0dfbe3aca5775
samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala 1f4c247e7a9918695619fd236c9ed4950ddbc967
Diff: https://reviews.apache.org/r/24011/diff/
Testing
-------
Thanks,
Chinmay Soman
Re: Review Request 24011: SAMZA-109: Make task.opts easier to use
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24011/#review48920
-----------------------------------------------------------
Looks good overall. Mostly nits.
Also, make sure to rebase. Jakob just committed SAMZA-123, which will conflict with this patch.
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala
<https://reviews.apache.org/r/24011/#comment85710>
Maybe name local-thread-task to differentiate from process job task name.
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala
<https://reviews.apache.org/r/24011/#comment85712>
This can be eliminated by just having a single line: Class.forName(...)
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala
<https://reviews.apache.org/r/24011/#comment85713>
This can be eliminated by just having a single line: new ShellCommandBuilder
samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala
<https://reviews.apache.org/r/24011/#comment85711>
Maybe name local-process-task to differentiate from thread job task name.
samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala
<https://reviews.apache.org/r/24011/#comment85708>
This line needs to be updated to recommend that the dev use ProcessJobFactory now.
samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala
<https://reviews.apache.org/r/24011/#comment85709>
nit: delete white space.
samza-shell/src/main/bash/run-class.sh
<https://reviews.apache.org/r/24011/#comment85707>
This chunk of code is a bit complex, and could benefit from some comments.
I think I get what you're doing: default to gc logging, 768MB heap, and turn on log rotation if possible. If task.opts is set, turn on gc logging if it's not already on, and turn on rotation if it's not already on.
This will force gc logging to always be on, I think (unless they set PrintGCDateStamps without loggc set). This should be OK for now. We can always add some way to fully disable gc logging if we want.
samza-shell/src/main/bash/run-class.sh
<https://reviews.apache.org/r/24011/#comment85705>
Seems like we can make this a function similar to enable_gc_log_rotation. Might be a little cleaner.
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala
<https://reviews.apache.org/r/24011/#comment85703>
I know this isn't your fault, but might be nice to do a getCanonicalName here, to make things easier to debug in the future.
samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala
<https://reviews.apache.org/r/24011/#comment85704>
Same deal here with getCanonicalName.
- Chris Riccomini
On July 28, 2014, 9:52 p.m., Chinmay Soman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24011/
> -----------------------------------------------------------
>
> (Updated July 28, 2014, 9:52 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-109
> https://issues.apache.org/jira/browse/SAMZA-109
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-109: Make task.opts easier to use
>
>
> Diffs
> -----
>
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala e20e7c1bcbc278bd61cf9446090b09d00744abb8
> samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala PRE-CREATION
> samza-shell/src/main/bash/run-am.sh 878d938329ee0956b8261aee35f73a31111a5223
> samza-shell/src/main/bash/run-class.sh a75cbb915659dcf69febe5bc91999225aa21971f
> samza-shell/src/main/bash/run-container.sh b43bc3f7e6d7d3df7b3edd8e910f5ac9de548295
> samza-test/src/main/resources/hello-stateful-world.samsa 8c2f2e47f77549a772d7d8d7cf922f7bfd1112b6
> samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala dc44a992083df6f70177aabfa8c0dfbe3aca5775
> samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala 1f4c247e7a9918695619fd236c9ed4950ddbc967
>
> Diff: https://reviews.apache.org/r/24011/diff/
>
>
> Testing
> -------
>
> Manual testing done by adding the following config to wikipedia-feed.properties (in hello-samza):
>
> task.opts=-Xmx600M
>
>
> (For both Yarn and Process JobFactory).
>
> The resulting config had this user property as well as all the other standard flags in JVM_OPTS: (GC related flags, log directory)
>
>
> Thanks,
>
> Chinmay Soman
>
>
Re: Review Request 24011: SAMZA-109: Make task.opts easier to use
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24011/#review49062
-----------------------------------------------------------
Two minor nits. Throw a quick patch up and I'll commit.
docs/learn/documentation/0.7.0/jobs/configuration-table.html
<https://reviews.apache.org/r/24011/#comment85886>
I think it defaults to this when YarnJobFactory is used, too. Perhaps we should just leave it to just define the default. I think it is only not used in ThreadJobFactory (right now).
docs/learn/documentation/0.7.0/jobs/packaging.md
<https://reviews.apache.org/r/24011/#comment85887>
Nit: no spaces around /
- Chris Riccomini
On July 29, 2014, 11:15 p.m., Chinmay Soman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24011/
> -----------------------------------------------------------
>
> (Updated July 29, 2014, 11:15 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-109
> https://issues.apache.org/jira/browse/SAMZA-109
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-109: Fixing ThreadJobFactory after the rebase
>
>
> SAMZA-109: Addressing review comments
>
>
> Modifying run-class.sh to add standard flags to user specified JVM_OPTS
>
>
> SAMZA:109 Make task.opts easier to use
>
>
> Diffs
> -----
>
> docs/learn/documentation/0.7.0/jobs/configuration-table.html 41f334f20ce88b425bcbda861c6ff5bd9194b8a8
> docs/learn/documentation/0.7.0/jobs/configuration.md 2ed3ea7d9d7e4aad92f5f077cf434a7eaa9a2cba
> docs/learn/documentation/0.7.0/jobs/job-runner.md 55c91144e5d8af2415a75dcf76f9d464fa920eb0
> docs/learn/documentation/0.7.0/jobs/packaging.md f888f7537bc47ee1de7bcc135bcc1bfa6b140628
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
> samza-shell/src/main/bash/run-am.sh 878d938329ee0956b8261aee35f73a31111a5223
> samza-shell/src/main/bash/run-class.sh a75cbb915659dcf69febe5bc91999225aa21971f
> samza-shell/src/main/bash/run-container.sh b43bc3f7e6d7d3df7b3edd8e910f5ac9de548295
> samza-test/src/main/resources/hello-stateful-world.samsa 8c2f2e47f77549a772d7d8d7cf922f7bfd1112b6
> samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala 3ed8b5cc841f592b8d9bc8007ce04720825b3634
> samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala 1f4c247e7a9918695619fd236c9ed4950ddbc967
>
> Diff: https://reviews.apache.org/r/24011/diff/
>
>
> Testing
> -------
>
> Manual testing done by adding the following config to wikipedia-feed.properties (in hello-samza):
>
> task.opts=-Xmx600M
>
>
> (For both Yarn and Process JobFactory).
>
> The resulting config had this user property as well as all the other standard flags in JVM_OPTS: (GC related flags, log directory)
>
>
> Thanks,
>
> Chinmay Soman
>
>
Re: Review Request 24011: SAMZA-109: Make task.opts easier to use
Posted by Naveen Somasundaram <na...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24011/#review49287
-----------------------------------------------------------
Ship it!
Ship It!
- Naveen Somasundaram
On July 31, 2014, 6:57 p.m., Chinmay Soman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24011/
> -----------------------------------------------------------
>
> (Updated July 31, 2014, 6:57 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-109
> https://issues.apache.org/jira/browse/SAMZA-109
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-109: Bug fix in ProcessJobFactory (introduced by SAMZA-123)
>
>
> SAMZA-109: Small fix in documentation
>
>
> SAMZA-109: Fixing ThreadJobFactory after the rebase
>
>
> SAMZA-109: Addressing review comments
>
>
> Modifying run-class.sh to add standard flags to user specified JVM_OPTS
>
>
> SAMZA:109 Make task.opts easier to use
>
>
> Diffs
> -----
>
> docs/learn/documentation/0.7.0/jobs/configuration-table.html 41f334f20ce88b425bcbda861c6ff5bd9194b8a8
> docs/learn/documentation/0.7.0/jobs/configuration.md 2ed3ea7d9d7e4aad92f5f077cf434a7eaa9a2cba
> docs/learn/documentation/0.7.0/jobs/job-runner.md 55c91144e5d8af2415a75dcf76f9d464fa920eb0
> docs/learn/documentation/0.7.0/jobs/packaging.md f888f7537bc47ee1de7bcc135bcc1bfa6b140628
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
> samza-shell/src/main/bash/run-am.sh 878d938329ee0956b8261aee35f73a31111a5223
> samza-shell/src/main/bash/run-class.sh a75cbb915659dcf69febe5bc91999225aa21971f
> samza-shell/src/main/bash/run-container.sh b43bc3f7e6d7d3df7b3edd8e910f5ac9de548295
> samza-test/src/main/resources/hello-stateful-world.samsa 8c2f2e47f77549a772d7d8d7cf922f7bfd1112b6
> samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala 3ed8b5cc841f592b8d9bc8007ce04720825b3634
> samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala 1f4c247e7a9918695619fd236c9ed4950ddbc967
>
> Diff: https://reviews.apache.org/r/24011/diff/
>
>
> Testing
> -------
>
> Manual testing done by adding the following config to wikipedia-feed.properties (in hello-samza):
>
> task.opts=-Xmx600M
>
>
> (For both Yarn and Process JobFactory).
>
> The resulting config had this user property as well as all the other standard flags in JVM_OPTS: (GC related flags, log directory)
>
>
> Thanks,
>
> Chinmay Soman
>
>
Re: Review Request 24011: SAMZA-109: Make task.opts easier to use
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24011/
-----------------------------------------------------------
(Updated July 31, 2014, 6:57 p.m.)
Review request for samza.
Bugs: SAMZA-109
https://issues.apache.org/jira/browse/SAMZA-109
Repository: samza
Description (updated)
-------
SAMZA-109: Bug fix in ProcessJobFactory (introduced by SAMZA-123)
SAMZA-109: Small fix in documentation
SAMZA-109: Fixing ThreadJobFactory after the rebase
SAMZA-109: Addressing review comments
Modifying run-class.sh to add standard flags to user specified JVM_OPTS
SAMZA:109 Make task.opts easier to use
Diffs (updated)
-----
docs/learn/documentation/0.7.0/jobs/configuration-table.html 41f334f20ce88b425bcbda861c6ff5bd9194b8a8
docs/learn/documentation/0.7.0/jobs/configuration.md 2ed3ea7d9d7e4aad92f5f077cf434a7eaa9a2cba
docs/learn/documentation/0.7.0/jobs/job-runner.md 55c91144e5d8af2415a75dcf76f9d464fa920eb0
docs/learn/documentation/0.7.0/jobs/packaging.md f888f7537bc47ee1de7bcc135bcc1bfa6b140628
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
samza-shell/src/main/bash/run-am.sh 878d938329ee0956b8261aee35f73a31111a5223
samza-shell/src/main/bash/run-class.sh a75cbb915659dcf69febe5bc91999225aa21971f
samza-shell/src/main/bash/run-container.sh b43bc3f7e6d7d3df7b3edd8e910f5ac9de548295
samza-test/src/main/resources/hello-stateful-world.samsa 8c2f2e47f77549a772d7d8d7cf922f7bfd1112b6
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala 3ed8b5cc841f592b8d9bc8007ce04720825b3634
samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala 1f4c247e7a9918695619fd236c9ed4950ddbc967
Diff: https://reviews.apache.org/r/24011/diff/
Testing
-------
Manual testing done by adding the following config to wikipedia-feed.properties (in hello-samza):
task.opts=-Xmx600M
(For both Yarn and Process JobFactory).
The resulting config had this user property as well as all the other standard flags in JVM_OPTS: (GC related flags, log directory)
Thanks,
Chinmay Soman
Re: Review Request 24011: SAMZA-109: Make task.opts easier to use
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24011/
-----------------------------------------------------------
(Updated July 29, 2014, 11:15 p.m.)
Review request for samza.
Bugs: SAMZA-109
https://issues.apache.org/jira/browse/SAMZA-109
Repository: samza
Description (updated)
-------
SAMZA-109: Fixing ThreadJobFactory after the rebase
SAMZA-109: Addressing review comments
Modifying run-class.sh to add standard flags to user specified JVM_OPTS
SAMZA:109 Make task.opts easier to use
Diffs (updated)
-----
docs/learn/documentation/0.7.0/jobs/configuration-table.html 41f334f20ce88b425bcbda861c6ff5bd9194b8a8
docs/learn/documentation/0.7.0/jobs/configuration.md 2ed3ea7d9d7e4aad92f5f077cf434a7eaa9a2cba
docs/learn/documentation/0.7.0/jobs/job-runner.md 55c91144e5d8af2415a75dcf76f9d464fa920eb0
docs/learn/documentation/0.7.0/jobs/packaging.md f888f7537bc47ee1de7bcc135bcc1bfa6b140628
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala 713bded5e53190f546d11ca66c0953028337e68d
samza-shell/src/main/bash/run-am.sh 878d938329ee0956b8261aee35f73a31111a5223
samza-shell/src/main/bash/run-class.sh a75cbb915659dcf69febe5bc91999225aa21971f
samza-shell/src/main/bash/run-container.sh b43bc3f7e6d7d3df7b3edd8e910f5ac9de548295
samza-test/src/main/resources/hello-stateful-world.samsa 8c2f2e47f77549a772d7d8d7cf922f7bfd1112b6
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala 3ed8b5cc841f592b8d9bc8007ce04720825b3634
samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala 1f4c247e7a9918695619fd236c9ed4950ddbc967
Diff: https://reviews.apache.org/r/24011/diff/
Testing
-------
Manual testing done by adding the following config to wikipedia-feed.properties (in hello-samza):
task.opts=-Xmx600M
(For both Yarn and Process JobFactory).
The resulting config had this user property as well as all the other standard flags in JVM_OPTS: (GC related flags, log directory)
Thanks,
Chinmay Soman
Re: Review Request 24011: SAMZA-109: Make task.opts easier to use
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24011/
-----------------------------------------------------------
(Updated July 28, 2014, 9:52 p.m.)
Review request for samza.
Bugs: SAMZA-109
https://issues.apache.org/jira/browse/SAMZA-109
Repository: samza
Description
-------
SAMZA-109: Make task.opts easier to use
Diffs
-----
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala e20e7c1bcbc278bd61cf9446090b09d00744abb8
samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala PRE-CREATION
samza-shell/src/main/bash/run-am.sh 878d938329ee0956b8261aee35f73a31111a5223
samza-shell/src/main/bash/run-class.sh a75cbb915659dcf69febe5bc91999225aa21971f
samza-shell/src/main/bash/run-container.sh b43bc3f7e6d7d3df7b3edd8e910f5ac9de548295
samza-test/src/main/resources/hello-stateful-world.samsa 8c2f2e47f77549a772d7d8d7cf922f7bfd1112b6
samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala dc44a992083df6f70177aabfa8c0dfbe3aca5775
samza-test/src/test/scala/org/apache/samza/test/performance/TestSamzaContainerPerformance.scala 1f4c247e7a9918695619fd236c9ed4950ddbc967
Diff: https://reviews.apache.org/r/24011/diff/
Testing (updated)
-------
Manual testing done by adding the following config to wikipedia-feed.properties (in hello-samza):
task.opts=-Xmx600M
(For both Yarn and Process JobFactory).
The resulting config had this user property as well as all the other standard flags in JVM_OPTS: (GC related flags, log directory)
Thanks,
Chinmay Soman