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