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/16 22:43:29 UTC
Review Request 23564: SAMZA-337: Compress Samza configuration passed to Yarn
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/
-----------------------------------------------------------
Review request for samza.
Bugs: SAMZA-337
https://issues.apache.org/jira/browse/SAMZA-337
Repository: samza
Description
-------
SAMZA-337: Compress Samza configuration passed to Yarn
Diffs
-----
build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
Diff: https://reviews.apache.org/r/23564/diff/
Testing
-------
Thanks,
Chinmay Soman
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/#review48026
-----------------------------------------------------------
I think my prior comments were dropped, since I RB'd twice. The latest patch seems to just introduce the isCompressed boolean.
Attaching the originals again.
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala
<https://reviews.apache.org/r/23564/#comment84273>
In keeping with existing config style, can we do task.config.compress?
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/23564/#comment84272>
Having a var named paramVal seems kind of odd. I think there is a slightly more idiomatic way of doing this. Simply have the if block return either Util.decompress(param)'s results as the last line, or return param itself.
val isCompressed = System.getenv(ShellCommandConfig.ENV_COMPRESS_CONFIG)
if (isCompressed.equals("TRUE")) {
info("Compression is ON !")
val decommpressedParam = Util.decompress(param)
info("Got param = " + decommpressedParam)
decommpressedParam
} else {
info("Parameter is uncompressed. Using it as-is")
param
}
- Chris Riccomini
On July 17, 2014, 5:46 p.m., Chinmay Soman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23564/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 5:46 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-337
> https://issues.apache.org/jira/browse/SAMZA-337
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-337: More review comments
>
>
> SAMZA-337: Addressing review comments
>
>
> SAMZA-337: Compress Samza configuration passed to Yarn
>
>
> Diffs
> -----
>
> build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
> gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
> samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
> samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
>
> Diff: https://reviews.apache.org/r/23564/diff/
>
>
> Testing
> -------
>
> Tested by adding this property to all the jobs in hello-samza:
>
> +config.compress=true
>
> After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
>
>
> Thanks,
>
> Chinmay Soman
>
>
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/
-----------------------------------------------------------
(Updated July 17, 2014, 6:05 p.m.)
Review request for samza.
Bugs: SAMZA-337
https://issues.apache.org/jira/browse/SAMZA-337
Repository: samza
Description (updated)
-------
SAMZA-337: More changes to address review comments
SAMZA-337: More review comments
SAMZA-337: Addressing review comments
SAMZA-337: Compress Samza configuration passed to Yarn
Diffs (updated)
-----
build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
Diff: https://reviews.apache.org/r/23564/diff/
Testing
-------
Tested by adding this property to all the jobs in hello-samza:
+config.compress=true
After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
Thanks,
Chinmay Soman
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/
-----------------------------------------------------------
(Updated July 17, 2014, 5:46 p.m.)
Review request for samza.
Bugs: SAMZA-337
https://issues.apache.org/jira/browse/SAMZA-337
Repository: samza
Description (updated)
-------
SAMZA-337: More review comments
SAMZA-337: Addressing review comments
SAMZA-337: Compress Samza configuration passed to Yarn
Diffs (updated)
-----
build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
Diff: https://reviews.apache.org/r/23564/diff/
Testing
-------
Tested by adding this property to all the jobs in hello-samza:
+config.compress=true
After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
Thanks,
Chinmay Soman
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/#review48014
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/23564/#comment84265>
Suggest moving ShellCommandConfig.ENV_COMPRESS_CONFIG out of this method, and taking a boolean parameter instead. Makes the method slightly more unit testable.
- Chris Riccomini
On July 17, 2014, 12:02 a.m., Chinmay Soman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23564/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 12:02 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-337
> https://issues.apache.org/jira/browse/SAMZA-337
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-337: Addressing review comments
>
>
> SAMZA-337: Compress Samza configuration passed to Yarn
>
>
> Diffs
> -----
>
> build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
> gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
> samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
> samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
>
> Diff: https://reviews.apache.org/r/23564/diff/
>
>
> Testing
> -------
>
> Tested by adding this property to all the jobs in hello-samza:
>
> +config.compress=true
>
> After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
>
>
> Thanks,
>
> Chinmay Soman
>
>
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/#review48013
-----------------------------------------------------------
Two minor nits. Other than that, looks good.
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala
<https://reviews.apache.org/r/23564/#comment84262>
In keeping with existing config style, can we do task.config.compress?
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/23564/#comment84264>
Having a var named paramVal seems kind of odd. I think there is a slightly more idiomatic way of doing this. Simply have the if bock return either Util.decompress(param)'s results as the last line, or return param itself.
val isCompressed = System.getenv(ShellCommandConfig.ENV_COMPRESS_CONFIG)
if (isCompressed.equals("TRUE")) {
info("Compression is ON !")
val decommpressedParam = Util.decompress(param)
info("Got param = " + decommpressedParam)
decommpressedParam
} else {
info("Parameter is uncompressed. Using it as-is")
param
}
- Chris Riccomini
On July 17, 2014, 12:02 a.m., Chinmay Soman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23564/
> -----------------------------------------------------------
>
> (Updated July 17, 2014, 12:02 a.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-337
> https://issues.apache.org/jira/browse/SAMZA-337
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-337: Addressing review comments
>
>
> SAMZA-337: Compress Samza configuration passed to Yarn
>
>
> Diffs
> -----
>
> build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
> gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
> samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
> samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
>
> Diff: https://reviews.apache.org/r/23564/diff/
>
>
> Testing
> -------
>
> Tested by adding this property to all the jobs in hello-samza:
>
> +config.compress=true
>
> After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
>
>
> Thanks,
>
> Chinmay Soman
>
>
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/
-----------------------------------------------------------
(Updated July 17, 2014, 12:02 a.m.)
Review request for samza.
Bugs: SAMZA-337
https://issues.apache.org/jira/browse/SAMZA-337
Repository: samza
Description (updated)
-------
SAMZA-337: Addressing review comments
SAMZA-337: Compress Samza configuration passed to Yarn
Diffs (updated)
-----
build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
Diff: https://reviews.apache.org/r/23564/diff/
Testing
-------
Tested by adding this property to all the jobs in hello-samza:
+config.compress=true
After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
Thanks,
Chinmay Soman
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/#review47957
-----------------------------------------------------------
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala
<https://reviews.apache.org/r/23564/#comment84205>
Remove whitespace just to keep config vars grouped together. Just stylistic.
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala
<https://reviews.apache.org/r/23564/#comment84203>
Can you move this by the other ENV variables up above? Just stylistic.
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/23564/#comment84207>
Move all of this logic into something like decompressConfig(configStr)
- Chris Riccomini
On July 16, 2014, 8:47 p.m., Chinmay Soman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23564/
> -----------------------------------------------------------
>
> (Updated July 16, 2014, 8:47 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-337
> https://issues.apache.org/jira/browse/SAMZA-337
>
>
> Repository: samza
>
>
> Description
> -------
>
> SAMZA-337: Compress Samza configuration passed to Yarn
>
>
> Diffs
> -----
>
> build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
> gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
> samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
> samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
>
> Diff: https://reviews.apache.org/r/23564/diff/
>
>
> Testing
> -------
>
> Tested by adding this property to all the jobs in hello-samza:
>
> +config.compress=true
>
> After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
>
>
> Thanks,
>
> Chinmay Soman
>
>
Re: Review Request 23564: SAMZA-337: Compress Samza configuration passed to
Yarn
Posted by Chinmay Soman <ch...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23564/
-----------------------------------------------------------
(Updated July 16, 2014, 8:47 p.m.)
Review request for samza.
Changes
-------
Manual testing done using hello-samza
Bugs: SAMZA-337
https://issues.apache.org/jira/browse/SAMZA-337
Repository: samza
Description
-------
SAMZA-337: Compress Samza configuration passed to Yarn
Diffs
-----
build.gradle 6d0ac973394bb0d733179ef954a973a06d024aee
gradle/dependency-versions.gradle 73735821426d5427cfbcdcb5fd86d8fe7ac53447
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala 4c2d365705bc21070dba8f2b889bcb68faccc962
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala b303615ca0978a92febe227fcf88b5acecce4223
samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala f8865b19b9afd3cab4ca7bbed4567ab800c4a648
samza-core/src/main/scala/org/apache/samza/util/Util.scala 11c23d0fdf2cd9daaac5b8e248acc51464d64d06
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala a67ecdf44af1a35d88b3d32dd2a05861bf728440
Diff: https://reviews.apache.org/r/23564/diff/
Testing (updated)
-------
Tested by adding this property to all the jobs in hello-samza:
+config.compress=true
After deploying the job, I see a compressed value in launch_container.sh. The job ran successfully in this case (also tested with config.compress=false)
Thanks,
Chinmay Soman