> On May 30, 2015, 8:58 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/LocalityManager.java, > > line 27 > > <https://reviews.apache.org/r/34746/diff/2/?file=974783#file974783line27> > > > > nit: why is here a "*" import?
not intentional :) > On May 30, 2015, 8:58 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/LocalityManager.java, > > line 62 > > <https://reviews.apache.org/r/34746/diff/2/?file=974783#file974783line62> > > > > This would be invoked twice by checkpointManager and localityManager. > > Generally, I felt that since checkpointManager and localityManager are > > referring to the same set of coordinatorSystemConsumer and > > coordinatorSystemProducer, shouldn't the start/stop/register be in the same > > life cycle, instead of being invoked twice separately? Yeah. Naveen and I discussed about this. CheckpointManager, ChangelogManager and LocalityManger can all use the same CoordinatorStreamAccessor instance and we can have accessors like getCoordinatorStreamProducer(), getCoordinatorStreamConsumer() interfaces. We have to refactor the entire set of coordinator stream related code. I started making these changes. But they are pretty extensive. This change will be a part of [SAMZA-678](https://issues.apache.org/jira/browse/SAMZA-678) > On May 30, 2015, 8:58 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/java/org/apache/samza/container/LocalityManager.java, > > line 79 > > <https://reviews.apache.org/r/34746/diff/2/?file=974783#file974783line79> > > > > What if the existingMapping == hostHttpAddress? I think that would be > > the "not moved" case? Ah.. you are right. I will add the equality check. > On May 30, 2015, 8:58 a.m., Yi Pan (Data Infrastructure) wrote: > > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala, > > line 74 > > <https://reviews.apache.org/r/34746/diff/2/?file=974787#file974787line74> > > > > Maybe, it is easier to wrap the three managers that all requires to > > initialize the coordinatorSystemProducer/coordinatorSystemConsumer in a > > single CoordinatorStreamManager? And > > CoordinatorStreamManager.getCheckpointManager()/getChangelogManager()/getLocalityManager() > > would return the specific management function handler? I guess this is also a pattern we can follow during the refactoring. As mentioned above, I will add this refactoring as a part of [SAMZA-678](https://issues.apache.org/jira/browse/SAMZA-678) - Navina ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34746/#review85855 ----------------------------------------------------------- On May 29, 2015, 11:26 p.m., Navina Ramesh wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34746/ > ----------------------------------------------------------- > > (Updated May 29, 2015, 11:26 p.m.) > > > Review request for samza, Chris Riccomini, Guozhang Wang, Yi Pan (Data > Infrastructure), and Naveen Somasundaram. > > > Bugs: SAMZA-618 > https://issues.apache.org/jira/browse/SAMZA-618 > > > Repository: samza > > > Description > ------- > > Adding Locality Manager file > > > reading in JC and writing from containers > > > After SAMZA-686 changes > > > Fixing stylechecks > > > Correcting when coordinator system accessors start & stop > > > Corrected documentation > > > Diffs > ----- > > checkstyle/import-control.xml 5f8e103a2e43f96518b20de1c7cbd84e0af24842 > samza-core/src/main/java/org/apache/samza/container/LocalityManager.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamMessage.java > 0988dedc3e8ad1b4080fb89dfff7c6f95fba8b67 > samza-core/src/main/java/org/apache/samza/job/model/JobModel.java > fa113e12080384586b329c82133bc0601b855ae5 > samza-core/src/main/scala/org/apache/samza/container/SamzaContainer.scala > 50e53fbcb55c4e9176bf29217a341b195c96d762 > samza-core/src/main/scala/org/apache/samza/coordinator/JobCoordinator.scala > 5b43b58a851c363846b433ebd589ce6dd5c5c932 > > samza-core/src/test/scala/org/apache/samza/container/TestSamzaContainer.scala > a7fa0857d1243f5a24e4550a39ee230fbd7705bb > > Diff: https://reviews.apache.org/r/34746/diff/ > > > Testing > ------- > > Used a sample job to test it locally and also, by setting up YARN on 3 > machines. > Verified that the message is correctly written and consumed from the > Coordinator Stream > > > Thanks, > > Navina Ramesh > >
