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