----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92825 -----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java (line 43) <https://reviews.apache.org/r/36545/#comment147063> To be consistent, lets go with TaskName, not the String. samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java (line 57) <https://reviews.apache.org/r/36545/#comment147064> Any reason that you do not want to use the TaskName class? TaskName seems fine here. samza-core/src/main/java/org/apache/samza/container/LocalityManager.java (line 50) <https://reviews.apache.org/r/36545/#comment147065> sourceSuffix is more descriptive. samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 20) <https://reviews.apache.org/r/36545/#comment147066> I think it makes sense that this class stays in its original package: samza-core/src/main/java/org/apache/samza/coordinator/stream . Because its only about the coordinatorStream, not the overall "manager" of the samza. samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 29) <https://reviews.apache.org/r/36545/#comment147071> a little more in the doc. This class is not really "manages" the coordinator stream, it is an abstract class that other stream managers want to extend. Also, renaming it to AbstractCoordinatorStreamManager maybe helpful too. samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 65) <https://reviews.apache.org/r/36545/#comment147072> typo, sends samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 96) <https://reviews.apache.org/r/36545/#comment147073> no "+" samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java (line 112) <https://reviews.apache.org/r/36545/#comment147074> I think, taskName maybe more general. In case we have more information in the TaskName, or other rules of registering. Just personal idea. samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java (line 52) <https://reviews.apache.org/r/36545/#comment147067> going with the taskName is fine. samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala (line 151) <https://reviews.apache.org/r/36545/#comment147068> if we use TaskName in the regitser method, do not need to change this one. samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala (line 245) <https://reviews.apache.org/r/36545/#comment147069> same samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala (line 185) <https://reviews.apache.org/r/36545/#comment147070> same - Yan Fang On July 16, 2015, 1:33 p.m., József Márton Jung wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36545/ > ----------------------------------------------------------- > > (Updated July 16, 2015, 1:33 p.m.) > > > Review request for samza. > > > Repository: samza > > > Description > ------- > > The following has been refactored: > 1. Static inner classes from CoordinatorStreamMessage has been extracted > 2. Common functionality from CheckpointManager, ChangelogMappingManager and > LocalityManager has benn moved to a base class > > > Diffs > ----- > > checkstyle/import-control.xml eef3370 > samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java > 7445996 > samza-core/src/main/java/org/apache/samza/container/LocalityManager.java > 55c258f > > samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java > 6bd1bd3 > > samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java > b1078bd > > samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemProducer.java > 92f8907 > > samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/CoordinatorStreamMessage.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/Delete.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetChangelogMapping.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetCheckpoint.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetConfig.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/messages/SetContainerHostMapping.java > PRE-CREATION > samza-core/src/main/java/org/apache/samza/job/model/JobModel.java ad6387d > > samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java > 7d3409c > samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala > 2e3aeb8 > samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala > 20e5d26 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > f621611 > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala 1c178a6 > > samza-core/src/test/java/org/apache/samza/coordinator/stream/MockCoordinatorStreamWrappedConsumer.java > e454593 > > samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamMessage.java > ac26a01 > > samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemConsumer.java > c25f6a7 > > samza-core/src/test/java/org/apache/samza/coordinator/stream/TestCoordinatorStreamSystemProducer.java > 68e3255 > > samza-core/src/test/scala/org/apache/samza/checkpoint/TestOffsetManager.scala > 8d54c46 > > samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala > 84fdeaa > samza-yarn/src/main/resources/scalate/WEB-INF/views/index.scaml 41303f7 > > Diff: https://reviews.apache.org/r/36545/diff/ > > > Testing > ------- > > Tests has been updated. > > > Thanks, > > József Márton Jung > >
