> On June 9, 2016, 7:15 p.m., Chris Pettitt wrote: > > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala, > > lines 620-622 > > <https://reviews.apache.org/r/48356/diff/2/?file=1411661#file1411661line620> > > > > I hope this is temporary. If not, if would be nicer to provide proper > > lifecycle / shutdown mechansisms. For example, on stop, I may want to flush > > a cache to disk. > > Navina Ramesh wrote: > Yes :) This is temporary. > Looking at the RunLoop which controls the lifecycle of tasks, we are > registering a shutdown handler. The way to trigger shutdown hook is to exit > the thread, right? Not sure how else to do it. > > Need to look into what value the shutdown hook brings to the runloop!
Xinyu is moving to the better lifecycle approach for RunLoop. It will now have a shutdown method instead of using a shutdown handler. > On June 9, 2016, 7:15 p.m., Chris Pettitt wrote: > > samza-core/src/main/java/org/apache/samza/coordinator/AbstractJobCoordinator.java, > > line 23 > > <https://reviews.apache.org/r/48356/diff/2/?file=1411653#file1411653line23> > > > > I'd probably ditch this. Over the years I've found that abstract base > > classes make things difficult to follow and also sometimes unnecessarily > > constrain you. They generally don't provide value that outweighs the cost. > > In this case the code here appears to be only used by one concrete class, > > so I'd definitely just inline it in one place, which makes following the > > code easier. > > > > When we get to the point we need more than one of these we can > > determine the best mechanism to share code (e.g. composition, external > > helper code) as appropriate. > > Navina Ramesh wrote: > I was hoping to introduce YarnJobCoordinator once Jagadish's work on > inversion and the standalone model was completely implemented. It should be > fine to get rid of this right now as I am not able to clearly define the > common functionalities across all the specific job coordinators. It does > simplify my current work. :) I'd suggest that we defer until we know exactly what we want to achieve and then we can pick the best tool for the job. > On June 9, 2016, 7:15 p.m., Chris Pettitt wrote: > > samza-core/src/main/java/org/apache/samza/container/grouper/stream/AllSspToSingleTaskGrouper.java, > > line 45 > > <https://reviews.apache.org/r/48356/diff/2/?file=1411649#file1411649line45> > > > > More directly: `return Collections.singleMap(taskName, ssps)`. > > Navina Ramesh wrote: > Got it. I think you mean singletonMap :) That's the one! :) > On June 9, 2016, 7:15 p.m., Chris Pettitt wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/ConfigBuilder.java, > > line 123 > > <https://reviews.apache.org/r/48356/diff/2/?file=1411641#file1411641line123> > > > > FWIW, when I have mandatory arguments to a builder I typically either > > take them in the constructor to the builder or take them as arguments to > > the build call. That works well when you have typed arguments - it becomes > > obvious at compile time that you need these values - but would not work > > well here where everything is a string. This defers checking to runtime. > > Navina Ramesh wrote: > Yes. My initial version took mandartory arguments in the constructor. > However, from the past Samza coding practices, once we add something to the > constructor, we don't keep a tab of them. Eventually, the number of mandatory > constructor arguments become really large. > Perhaps it is time to stop that practice! > > I can change the constructor to take the arguments and see if we can find > a way to control it :) I suspect the problem may come from having monolithic config builders. If we can keep them more focused I think the constructor based approach can be kept reasonable and it gives you a nicer experience with build-time failure. > 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. You could make the fields protected which at least reduces the scope of accessibility. Using setters would allow for more flexibility though. > On June 9, 2016, 7:15 p.m., Chris Pettitt wrote: > > samza-core/src/main/java/org/apache/samza/configbuilder/BuilderInterface.java, > > line 23 > > <https://reviews.apache.org/r/48356/diff/2/?file=1411639#file1411639line23> > > > > This doesn't seem like a particularly useful interface as it is > > untyped. Anywhere you could take this interface it would be possible to > > pass in the wrong type of builder. > > Navina Ramesh wrote: > The alternative I thought was to return an immutable Config object. But I > doubt if that gives us a lot of benefits. Are you suggesting that we don't > follow the Builder pattern or simply that this interface is not required? My suggestion was to remove the interface. I don't think you want to mix and match variants of BuilderInterface. If that assertion is wrong then it would help to discuss so I understand the motivation better, - Chris ----------------------------------------------------------- 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 > >
