You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@samza.apache.org by Jakob Homan <jg...@apache.org> on 2013/12/11 22:00:11 UTC
Review Request 16194: SAMZA-82
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16194/
-----------------------------------------------------------
Review request for samza.
Bugs: SAMZA-82
https://issues.apache.org/jira/browse/SAMZA-82
Repository: samza
Description
-------
Fix Samza-82
Diffs
-----
samza-api/src/main/java/org/apache/samza/job/CommandBuilder.java 934423b
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala b6efe46
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 9c4370f
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala aefec27
samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala c67e46d
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala ddb119b
samza-core/src/main/scala/org/apache/samza/util/Util.scala 6b2ec49
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala PRE-CREATION
samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala bc94f6a
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala 9f4db17
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala ee3ffef
Diff: https://reviews.apache.org/r/16194/diff/
Testing
-------
unit and manual
Thanks,
Jakob Homan
Re: Review Request 16194: SAMZA-82
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16194/#review30305
-----------------------------------------------------------
Ship it!
- Chris Riccomini
On Dec. 12, 2013, 11:19 p.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16194/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2013, 11:19 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-82
> https://issues.apache.org/jira/browse/SAMZA-82
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fix Samza-82
>
>
> Diffs
> -----
>
> samza-api/src/main/java/org/apache/samza/job/CommandBuilder.java 934423b
> samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala b6efe46
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 9c4370f
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala aefec27
> samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala c67e46d
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala ddb119b
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 6b2ec49
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala PRE-CREATION
> samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala bc94f6a
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala 9f4db17
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala ee3ffef
>
> Diff: https://reviews.apache.org/r/16194/diff/
>
>
> Testing
> -------
>
> unit and manual
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 16194: SAMZA-82
Posted by Jakob Homan <jg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16194/
-----------------------------------------------------------
(Updated Dec. 12, 2013, 3:19 p.m.)
Review request for samza.
Bugs: SAMZA-82
https://issues.apache.org/jira/browse/SAMZA-82
Repository: samza
Description
-------
Fix Samza-82
Diffs (updated)
-----
samza-api/src/main/java/org/apache/samza/job/CommandBuilder.java 934423b
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala b6efe46
samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 9c4370f
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala aefec27
samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala c67e46d
samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala ddb119b
samza-core/src/main/scala/org/apache/samza/util/Util.scala 6b2ec49
samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala PRE-CREATION
samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala bc94f6a
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala 9f4db17
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala ee3ffef
Diff: https://reviews.apache.org/r/16194/diff/
Testing
-------
unit and manual
Thanks,
Jakob Homan
Re: Review Request 16194: SAMZA-82
Posted by Jakob Homan <jg...@apache.org>.
> On Dec. 11, 2013, 3:33 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala, line 32
> > <https://reviews.apache.org/r/16194/diff/1/?file=396745#file396745line32>
> >
> > There might be some docs that mention this environment variable. Probably worth a grep to replace with the new name.
Did. Didn't see any.
> On Dec. 11, 2013, 3:33 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, line 339
> > <https://reviews.apache.org/r/16194/diff/1/?file=396747#file396747line339>
> >
> > Stylistic: strip extra white space line.
done
> On Dec. 11, 2013, 3:33 p.m., Chris Riccomini wrote:
> > samza-core/src/main/scala/org/apache/samza/util/Util.scala, line 134
> > <https://reviews.apache.org/r/16194/diff/1/?file=396750#file396750line134>
> >
> > I think this might be a misnomer that is not your fault, but that you inherited from my poor naming conventions in the Samza YARN code.
> >
> > What this method is actually used for, as far as I can tell, is to figure out which input SSPs a given samza CONTAINER is responsible for processing (not a task). It's unfortunate, but Samza's YARN code still refers to a container as a task. Therefore, taskId is an id for a YARN container, and taskCount is the total number of containers (yarn.container.count).
> >
> > Your call whether you want to stick with existing convention, update all mentions in the YARN AM to refer to container instead of task, or just update this chunk of code to do the right thing.
Changed it on this method (and method params). We can update the rest in another JIRA.
> On Dec. 11, 2013, 3:33 p.m., Chris Riccomini wrote:
> > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala, line 130
> > <https://reviews.apache.org/r/16194/diff/1/?file=396753#file396753line130>
> >
> > This is all using the wrong terminology. Should be "container" not "task". See comment above. Up to you if you want to fix, or open a new JIRA, or what.
done.
> On Dec. 11, 2013, 3:33 p.m., Chris Riccomini wrote:
> > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala, line 136
> > <https://reviews.apache.org/r/16194/diff/1/?file=396753#file396753line136>
> >
> > Is this actually used anywhere? The semantics have changed. Before, partitions.size would give you the count of all unique partitions (e.g. two input streams with 4 partitions each = 4 partitions).
> >
> > With your change, the total partitions now means the total number of SSPs that the container is processing (e.g. two input streams with 4 partitions each = 8 partitions).
No. Had meant to remove it. Done now.
- Jakob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16194/#review30233
-----------------------------------------------------------
On Dec. 11, 2013, 1 p.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16194/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2013, 1 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-82
> https://issues.apache.org/jira/browse/SAMZA-82
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fix Samza-82
>
>
> Diffs
> -----
>
> samza-api/src/main/java/org/apache/samza/job/CommandBuilder.java 934423b
> samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala b6efe46
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 9c4370f
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala aefec27
> samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala c67e46d
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala ddb119b
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 6b2ec49
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala PRE-CREATION
> samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala bc94f6a
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala 9f4db17
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala ee3ffef
>
> Diff: https://reviews.apache.org/r/16194/diff/
>
>
> Testing
> -------
>
> unit and manual
>
>
> Thanks,
>
> Jakob Homan
>
>
Re: Review Request 16194: SAMZA-82
Posted by Chris Riccomini <cr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16194/#review30233
-----------------------------------------------------------
Overall, looks pretty good. Few minor nits.
One other comment: IntelliJ seems to be inserting white space on new lines. Most of Samza's code doesn't have this since Eclipse does the opposite, by default. Kind of annoying to have both, and RB is highlighting all the spacing in red.
samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala
<https://reviews.apache.org/r/16194/#comment57876>
There might be some docs that mention this environment variable. Probably worth a grep to replace with the new name.
samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala
<https://reviews.apache.org/r/16194/#comment57877>
Stylistic: strip extra white space line.
samza-core/src/main/scala/org/apache/samza/util/Util.scala
<https://reviews.apache.org/r/16194/#comment57879>
I think this might be a misnomer that is not your fault, but that you inherited from my poor naming conventions in the Samza YARN code.
What this method is actually used for, as far as I can tell, is to figure out which input SSPs a given samza CONTAINER is responsible for processing (not a task). It's unfortunate, but Samza's YARN code still refers to a container as a task. Therefore, taskId is an id for a YARN container, and taskCount is the total number of containers (yarn.container.count).
Your call whether you want to stick with existing convention, update all mentions in the YARN AM to refer to container instead of task, or just update this chunk of code to do the right thing.
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala
<https://reviews.apache.org/r/16194/#comment57880>
This is all using the wrong terminology. Should be "container" not "task". See comment above. Up to you if you want to fix, or open a new JIRA, or what.
samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala
<https://reviews.apache.org/r/16194/#comment57881>
Is this actually used anywhere? The semantics have changed. Before, partitions.size would give you the count of all unique partitions (e.g. two input streams with 4 partitions each = 4 partitions).
With your change, the total partitions now means the total number of SSPs that the container is processing (e.g. two input streams with 4 partitions each = 8 partitions).
- Chris Riccomini
On Dec. 11, 2013, 9 p.m., Jakob Homan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16194/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2013, 9 p.m.)
>
>
> Review request for samza.
>
>
> Bugs: SAMZA-82
> https://issues.apache.org/jira/browse/SAMZA-82
>
>
> Repository: samza
>
>
> Description
> -------
>
> Fix Samza-82
>
>
> Diffs
> -----
>
> samza-api/src/main/java/org/apache/samza/job/CommandBuilder.java 934423b
> samza-core/src/main/scala/org/apache/samza/config/ShellCommandConfig.scala b6efe46
> samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala 9c4370f
> samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala aefec27
> samza-core/src/main/scala/org/apache/samza/job/ShellCommandBuilder.scala c67e46d
> samza-core/src/main/scala/org/apache/samza/job/local/LocalJobFactory.scala ddb119b
> samza-core/src/main/scala/org/apache/samza/util/Util.scala 6b2ec49
> samza-core/src/test/scala/org/apache/samza/util/TestUtil.scala PRE-CREATION
> samza-kafka/src/main/scala/org/apache/samza/checkpoint/kafka/KafkaCheckpointManagerFactory.scala bc94f6a
> samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterTaskManager.scala 9f4db17
> samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMasterTaskManager.scala ee3ffef
>
> Diff: https://reviews.apache.org/r/16194/diff/
>
>
> Testing
> -------
>
> unit and manual
>
>
> Thanks,
>
> Jakob Homan
>
>