> On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java, > > line 46 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013337#file1013337line46> > > > > To be consistent, lets go with TaskName, not the String.
Corrected. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/checkpoint/CheckpointManager.java, > > line 74 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013337#file1013337line74> > > > > Any reason that you do not want to use the TaskName class? TaskName > > seems fine here. Since the TaskName class consists of only one field called taskName, I tought that using only a string is sufficient. Chaned back to TaskName. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/container/LocalityManager.java, > > line 64 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013338#file1013338line64> > > > > sourceSuffix is more descriptive. Corrected. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, > > line 20 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line20> > > > > 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. Corrected, moved the class to samza-core/src/main/java/org/apache/samza/coordinator/stream package. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, > > line 29 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line29> > > > > 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. Renamed. Also Javadoc updated on the class. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, > > line 65 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line65> > > > > typo, sends Corrected. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, > > line 96 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line96> > > > > no "+" Removed. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/manager/CoordinatorStreamManager.java, > > line 112 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013349#file1013349line112> > > > > I think, taskName maybe more general. In case we have more information > > in the TaskName, or other rules of registering. Just personal idea. I agree. Renamed to taskName. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/java/org/apache/samza/storage/ChangelogPartitionManager.java, > > line 74 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013350#file1013350line74> > > > > going with the taskName is fine. Corrected. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/scala/org/apache/samza/checkpoint/CheckpointTool.scala, > > line 151 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013351#file1013351line151> > > > > if we use TaskName in the regitser method, do not need to change this > > one. Corrected. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/scala/org/apache/samza/checkpoint/OffsetManager.scala, > > line 245 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013352#file1013352line245> > > > > same Corrected. > On July 23, 2015, 10:35 p.m., Yan Fang wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 185 > > <https://reviews.apache.org/r/36545/diff/1/?file=1013353#file1013353line185> > > > > same Corrected. - József Márton ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36545/#review92825 ----------------------------------------------------------- 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 > >
