> On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/config/TaskConfigJava.java, line > > 131 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441777#file1441777line131> > > > > [INFO Question] Just curious, Is there a required code convention for > > samza code base here? Do we do the following pattern? > > > > if (....) { > > ....; > > }
I don't believe we have been strict on coding convention. The convention you suggested is what, I believe, is used at LinkedIn. We are not very stric on open-source. As long as the code is readable, it is ok. > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java, > > line 92 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441779#file1441779line92> > > > > Maybe if(!StringUtils.isBlank())? Since we would like to have a valid > > name. Is StringUtils part of java.lang? Or are you referring to some other library? > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java, > > line 144 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441779#file1441779line144> > > > > Would it be more accurate to call it STREAM_KEY_SERDE_FORMATTING_STRING > > instead of STREAM_KEY_SERDE_REGEX? regex has a different rule. Yeah makes sense! > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java, > > line 154 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441779#file1441779line154> > > > > Or just use StringUtils.isEmpty(streamName) or even > > StringUtils.isBlank(streamName)? Is StringUtils part of java.lang? Or are you referring to some other library? > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/KafkaCheckpointConfig.java, > > line 28 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441781#file1441781line28> > > > > [Info Question] what is the reason to have 3 and 26214400 as default > > values here? Replication factor indicates the number of replicas for the data in checkpoint topic. 3 is usually the recommended replication factor. Not sure about the segment bytes - 26214400. This will be a good question for the Kafka team > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/KafkaCheckpointConfig.java, > > line 59 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441781#file1441781line59> > > > > Why do we have systemName here since we have systemConfig in the result > > map which has systemName already? systemname serves as an indirection for the set of properties associated with a systemName. Systemname is essentially an identifier for the set of properties. Think of task.checkpoint.system as a "pointer". Samza configurations have some standard patterns. A complete list is available here - http://samza.apache.org/learn/documentation/latest/jobs/configuration-table.html > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/KafkaSystemConfig.java, > > line 26 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441782#file1441782line26> > > > > Similar to a previous comment. Maybe more proper to call it > > FORMATTING_STRING instead of REGEX? Makes sense! > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/SerdeConfig.java, > > line 43 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441783#file1441783line43> > > > > Could serdeAlias be null accidentally? If so, it would be better to > > have a null check for serdeAlias. SerdeAlias is an enum. I don't think you can specify a "null" value for enum. > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/container/grouper/stream/AllSspToSingleTaskGrouper.java, > > line 34 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441786#file1441786line34> > > > > Could ssps be null? ssps cannot be null. But doesn't hurt to check for it and avoid some random NPE. I will fix it. Thanks for noticing! > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/java/org/apache/samza/processor/StreamProcessor.java, > > line 166 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441793#file1441793line166> > > > > Would it be better to log separately for these two exceptions for > > trouble shooting purpose if issues happen? Not much can be inferrred from InterruptedException and ExecutionException. So, the log message is going to be the same. Didn't think it was necessary to duplicate log line in another code block as I felt it didn't add much value. > On July 12, 2016, 12:15 a.m., Fred Ji wrote: > > samza-core/src/main/scala/org/apache/samza/config/TaskConfig.scala, line 29 > > <https://reviews.apache.org/r/48356/diff/7/?file=1441797#file1441797line29> > > > > I know it is from previous code, but do you mind explaining or putting > > a comment regarding what -1L means here? Argh. It is kind of obvious to me. I can add some documentation here! :) negative value basically means window() method is never called. This configuration serves dual purpose - flag that indicates if window() is enabled or not AND the window time interval in ms. - Navina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48356/#review141799 ----------------------------------------------------------- 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 > >
