> On Oct. 5, 2016, 12:06 a.m., Jagadish Venkatraman wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java, > > line 29 > > <https://reviews.apache.org/r/47994/diff/2/?file=1519795#file1519795line29> > > > > nit: Probably an IDE thing. We can be more specific about imports.
Sounds good to me. Fixed. > On Oct. 5, 2016, 12:06 a.m., Jagadish Venkatraman wrote: > > samza-operator/src/main/java/org/apache/samza/operators/impl/ChainedOperators.java, > > line 43 > > <https://reviews.apache.org/r/47994/diff/2/?file=1519796#file1519796line43> > > > > This is not a correctness issue. > > > > Often, Wouldn't it be nice to do traversal of the operator chain the > > same as that of the insertion order? This is mainly to keep unique instance of OperatorImpl to avoid double invocation of the same downstream subscriber. IMO, the order of execution among all downstream subscribers does not seem to affect the correction. > On Oct. 5, 2016, 12:06 a.m., Jagadish Venkatraman wrote: > > samza-operator/src/main/java/org/apache/samza/operators/impl/ChainedOperators.java, > > line 74 > > <https://reviews.apache.org/r/47994/diff/2/?file=1519796#file1519796line74> > > > > This part was not obvious to me on how objects were being re-used. > > > > Can you give me an example when this `if` will be true? As discussed offline, added comments to explain. > On Oct. 5, 2016, 12:06 a.m., Jagadish Venkatraman wrote: > > samza-operator/src/main/java/org/apache/samza/operators/impl/OperatorFactory.java, > > line 55 > > <https://reviews.apache.org/r/47994/diff/2/?file=1519797#file1519797line55> > > > > nit: > > Please add a javadoc, on what the returned `Entry` is, and what the > > boolean is? Yes, fixed. > On Oct. 5, 2016, 12:06 a.m., Jagadish Venkatraman wrote: > > samza-operator/src/main/java/org/apache/samza/operators/impl/OperatorFactory.java, > > line 56 > > <https://reviews.apache.org/r/47994/diff/2/?file=1519797#file1519797line56> > > > > Wouldn't it be ideal if the user of this factory did not have to care > > about whether an object was created the first time versus the object was > > already created by the factory? The issue is, to avoid create multiple instances of the same operator in a join or merge case, we will have to know whether we should stop traversing the DAG for initialization or not. There might be another way to initialize the whole messageStreamToOperators at once to the operatorMap. Let me think it over. > On Oct. 5, 2016, 12:06 a.m., Jagadish Venkatraman wrote: > > samza-operator/src/main/java/org/apache/samza/operators/impl/OperatorFactory.java, > > line 58 > > <https://reviews.apache.org/r/47994/diff/2/?file=1519797#file1519797line58> > > > > Why do you need a check on `putIfAbsent` here given the `containsKey` > > check? It seems redundant. > > > > (IIUC, this class is not meant to be used in a multi-threaded mode. > > It's called from `init` which is single-threaded.) I would prefer to make the class thread-safe now, just in case that we will need to invoke the methods in multi-threaded environment. Besides, I don't think using CHM here incurs too much cost. > On Oct. 5, 2016, 12:06 a.m., Jagadish Venkatraman wrote: > > samza-operator/src/main/java/org/apache/samza/operators/impl/OperatorImpl.java, > > line 90 > > <https://reviews.apache.org/r/47994/diff/2/?file=1519798#file1519798line90> > > > > Do we also need the `offset` of the incoming message that flows through > > each of these operators? > > > > Ideally, the offset should be a part of the context.(since, this RB is > > just for the wire-up, I'm certainly open to doing it later.) You are right on spot. I think the offset+context should be in a separate RB. - Yi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47994/#review151356 ----------------------------------------------------------- On Oct. 4, 2016, 8:05 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. 4, 2016, 8:05 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) > >
