----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48356/#review141799 -----------------------------------------------------------
Thanks a lot for the RB! I only have a few INFO questions to learn more details and MINOR comments as below. Thanks! samza-core/src/main/java/org/apache/samza/config/TaskConfigJava.java (line 131) <https://reviews.apache.org/r/48356/#comment207185> [INFO Question] Just curious, Is there a required code convention for samza code base here? Do we do the following pattern? if (....) { ....; } samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java (line 92) <https://reviews.apache.org/r/48356/#comment207186> Maybe if(!StringUtils.isBlank())? Since we would like to have a valid name. samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java (line 144) <https://reviews.apache.org/r/48356/#comment207188> Would it be more accurate to call it STREAM_KEY_SERDE_FORMATTING_STRING instead of STREAM_KEY_SERDE_REGEX? regex has a different rule. samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java (line 154) <https://reviews.apache.org/r/48356/#comment207187> Or just use StringUtils.isEmpty(streamName) or even StringUtils.isBlank(streamName)? samza-core/src/main/java/org/apache/samza/configbuilder/KafkaCheckpointConfig.java (line 28) <https://reviews.apache.org/r/48356/#comment207190> [Info Question] what is the reason to have 3 and 26214400 as default values here? samza-core/src/main/java/org/apache/samza/configbuilder/KafkaCheckpointConfig.java (line 59) <https://reviews.apache.org/r/48356/#comment207191> Why do we have systemName here since we have systemConfig in the result map which has systemName already? samza-core/src/main/java/org/apache/samza/configbuilder/KafkaSystemConfig.java (line 26) <https://reviews.apache.org/r/48356/#comment207192> Similar to a previous comment. Maybe more proper to call it FORMATTING_STRING instead of REGEX? samza-core/src/main/java/org/apache/samza/configbuilder/SerdeConfig.java (line 43) <https://reviews.apache.org/r/48356/#comment207193> Could serdeAlias be null accidentally? If so, it would be better to have a null check for serdeAlias. samza-core/src/main/java/org/apache/samza/container/grouper/stream/AllSspToSingleTaskGrouper.java (line 34) <https://reviews.apache.org/r/48356/#comment207194> Could ssps be null? samza-core/src/main/java/org/apache/samza/processor/StreamProcessor.java (line 166) <https://reviews.apache.org/r/48356/#comment207195> Would it be better to log separately for these two exceptions for trouble shooting purpose if issues happen? samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala (line 29) <https://reviews.apache.org/r/48356/#comment207196> I know it is from previous code, but do you mind explaining or putting a comment regarding what -1L means here? - Fred Ji On July 12, 2016, midnight, Navina Ramesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48356/ > ----------------------------------------------------------- > > (Updated July 12, 2016, midnight) > > > Review request for samza, Chris Pettitt and Yi Pan (Data Infrastructure). > > > Repository: samza > > > Description > ------- > > Added ConfigBuilder and support classes > > Added JobCoordinator interfaces > > > Adding StreamProcessor, StandaloneJobCoordinator and updating SamzaContainer > interface > > > Added TestStreamProcessor and some unit tests for ConfigBuilders > > > Changing who defined processorId > > > Fixed checkstyle errors > > > Replaced SamzaException with ConfigException > > > Removing localityManager instantiation from Samza Container > > > Diffs > ----- > > build.gradle ba4a9d14fe24e1ff170873920cd5eeef656955af > checkstyle/import-control.xml 325c38131047836dc8aedaea4187598ef3ba7666 > samza-core/src/main/java/org/apache/samza/config/TaskConfigJava.java > 021d42a70179f5d14f51ac87cb09dcc97218095e > > samza-core/src/main/java/org/apache/samza/configbuilder/CheckpointConfig.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/configbuilder/GenericConfigBuilder.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/configbuilder/KafkaCheckpointConfig.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/configbuilder/KafkaSystemConfig.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/configbuilder/SerdeConfig.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/configbuilder/StandaloneConfigBuilder.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/configbuilder/SystemConfig.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/grouper/stream/AllSspToSingleTaskGrouper.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/grouper/stream/AllSspToSingleTaskGrouperFactory.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/grouper/task/SingleContainerGrouper.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/container/grouper/task/SingleContainerGrouperFactory.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/coordinator/JobCoordinator.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/JobCoordinatorFactory.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/JobModelUpdateHandler.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/processor/StreamProcessor.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/standalone/StandaloneJobCoordinator.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/standalone/StandaloneJobCoordinatorFactory.java > PRE-CREATION > samza-core/src/main/scala/org/apache/samza/config/JobConfig.scala > 49b08f6b68dbb44757dcc8ce8d60c365a9d22981 > samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala > 08a4debb06f9925ae741049abb2ee0df97b2243b > samza-core/src/main/scala/org/apache/samza/container/RunLoop.scala > cf05c15c836ddfa54ba8fe27abc18ed88ac5fc11 > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 18c09224bbae959342daf9b2b7a7d971cc224f48 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > d3bd9b7c11afd44ccfb681b660fefffafd216c29 > samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala > 56881d46be9f859999adabbbda20433b208e012e > > samza-core/src/test/java/org/apache/samza/configbuilder/TestStandaloneConfigBuilder.java > PRE-CREATION > samza-test/src/test/java/org/apache/samza/processor/MyStreamTask.java > PRE-CREATION > > samza-test/src/test/java/org/apache/samza/processor/TestStreamProcessor.java > PRE-CREATION > samza-yarn/src/main/java/org/apache/samza/config/YarnConfig.java > 8f2dc4853a2b5dd712f25a2d2d16402bcba89d7a > samza-yarn/src/main/java/org/apache/samza/job/yarn/SamzaTaskManager.java > bc95f31c0dcaaa68d483a6f152b61aba6c543fff > > Diff: https://reviews.apache.org/r/48356/diff/ > > > Testing > ------- > > ./gradlew clean build > > Local integration test: > ./bin/grid start zookeeper > ./bin/grid start kafka > Then, run TestStreamProcessor.java > > > Thanks, > > Navina Ramesh > >
