----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47994/#review151818 -----------------------------------------------------------
samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java (line 61) <https://reviews.apache.org/r/47994/#comment220362> 3 thoughts on this line: 1. Why should this be static? Wouldn't this preclude you from having two tasks run the same operator DAG in the same container/process? 2. And why here instead of the MessageStream or ChainedOperators classes? I would expect the topology to be an instantiated thing rather than a global map. At a minimum since this map and ChainedOperators encode similar information (subscribers to an operator or message stream) they should be consolidated to one source of truth for structural/topology info. 3. Does the order of the Operators in the list have any meaning? e.g. does it implicitly define the order of processing, or is it just for consistency, or is the List used to allow duplicates? samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java (line 78) <https://reviews.apache.org/r/47994/#comment220361> This can be simplified using the new java 8 "getOrDefault" method for maps. Also, should it really be null if there are no subscribers or Collections.emptyList()? samza-operator/src/main/java/org/apache/samza/operators/impl/OperatorFactory.java (line 42) <https://reviews.apache.org/r/47994/#comment220502> Another global map. We should be super clear about why these are being used and what the assumptions are. This type of code can be very fragile if we're assuming singletons and that assumption is later broken. - Jake Maes On Oct. 5, 2016, 7:50 a.m., Yi Pan (Data Infrastructure) wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47994/ > ----------------------------------------------------------- > > (Updated Oct. 5, 2016, 7:50 a.m.) > > > Review request for samza, Boris Shkolnik, Chris Pettitt, Chinmay Soman, Jake > Maes, Navina Ramesh, Jagadish Venkatraman, and Xinyu Liu. > > > Bugs: SAMZA-915 > https://issues.apache.org/jira/browse/SAMZA-915 > > > Repository: samza > > > Description > ------- > > SAMZA-915: implementation of StreamPipeline and operator runtime impl classes > > > Diffs > ----- > > > samza-operator/src/main/java/org/apache/samza/operators/api/MessageStream.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/ChainedOperators.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/OperatorFactory.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/OperatorImpl.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/join/PartialJoinOpImpl.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/window/SessionWindowImpl.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/api/TestMessageStream.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/api/internal/TestOperators.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/TestChainedOperators.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/TestOperatorFactory.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/TestOperatorImpl.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/TestOutputMessage.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/TestSimpleOperatorImpl.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/TestSinkOperatorImpl.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/data/serializers/SqlAvroSerdeTest.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/impl/window/TestSessionWindowImpl.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/task/BroadcastOperatorTask.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/task/InputJsonSystemMessage.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47994/diff/ > > > Testing > ------- > > ./gradlew clean build. > > > Thanks, > > Yi Pan (Data Infrastructure) > >
