> On June 9, 2016, 7:15 p.m., Chris Pettitt wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java, > > lines 42-53 > > <https://reviews.apache.org/r/48356/diff/2/?file=1411641#file1411641line42> > > > > Can these be private vs. package private? > > Navina Ramesh wrote: > I think some of them - jobCoordinatorFactory, sspGrouperFactory, > taskNameGrouperFactory - have to be package-private or protected so that > "standaloneConfigBuilder" or "GenericConfigBuilder" can impose the correct > values. An alternative is to make them private and provide accessor methods > with "protected" scope. Does that sound reasonable? > For now, the rest of the configs can have private access. > > Chris Pettitt wrote: > You could make the fields protected which at least reduces the scope of > accessibility. Using setters would allow for more flexibility though.
Thinking about it, if I make them "private" and provide setter, it will impossible to NOT expose these methods through the extended configbuilder classes. For example, if a user wants to use StandaloneConfigBuilder, the user shouldn't see an option for "setJobCoordinatorFactory" as it is pre-determined. I will leave them as protected for now and see how other changes impact the access pattern. The altenative is to move these configs to the derived class. But that might lead to some amount of redundancy in the build() method of the derived classes. I wasn't entirely clear on the right choice here and hence, I went with what felt more usable and intuitive for the user. - Navina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/48356/#review136871 ----------------------------------------------------------- On June 8, 2016, 9:59 p.m., Navina Ramesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/48356/ > ----------------------------------------------------------- > > (Updated June 8, 2016, 9:59 p.m.) > > > Review request for samza and Chris Pettitt. > > > 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 16facbbf4dff378c561461786ff186bd9e0000ed > checkstyle/import-control.xml 7a09c7ed8ab372d2342f31e850ae09c605292eb2 > > samza-core/src/main/java/org/apache/samza/configbuilder/BuilderInterface.java > PRE-CREATION > > 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/configbuilder/SystemStreamConfig.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/AbstractJobCoordinator.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 > 4d5ca4d3d2dd1542c5d073dfe6c13666ef5f51fc > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 951d479606423c39d600d69025c940af1f0f0600 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > e9a51083aff4dc316e94144f6242fe702ca73a68 > samza-core/src/main/scala/org/apache/samza/job/local/ThreadJobFactory.scala > 56881d46be9f859999adabbbda20433b208e012e > > samza-core/src/test/java/org/apache/samza/configbuilder/TestGenericConfigBuilder.java > PRE-CREATION > > 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 > c556d833059826d97ad6928de47330effba40219 > 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 > >
