> On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/MessageStream.java, > > line 53 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498252#file1498252line53> > > > > Minor: you have inline other transform functions below, so it's not > > obvious why this was not inlined. Inlined might be slightly nicer (for > > review at least :)) as it keeps the code in one place. Your call.
Make sense. I added this simply for code re-use purpose. Since there are not many places referring to this function, inline makes sense. > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/MessageStream.java, > > line 67 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498252#file1498252line67> > > > > Doc and code do not agree. The code has unbounded type parameters, > > while the doc suggests specific types. > > > > If the doc is correct, we should be able to satisfy it without a > > generic type. Good catch! Thanks for pointing out this. Will fix and do a scrub through the code for doc/code consistency. > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/Triggers.java, > > line 30 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498254#file1498254line30> > > > > It's probably worth noting that if there are multiple triggers the > > aggregate value is the disjunction of the individual values. Makes sense. Will add to the doc. Thanks! > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/Triggers.java, > > line 39 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498254#file1498254line39> > > > > Maybe TriggerBuilder as this differs a bit from the typical class of > > static factory methods implied by the class name? Sure. This is actually a builder. > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/Windows.java, > > line 48 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498256#file1498256line48> > > > > Does the concrete class need to be exposed? Interfaces are generally > > nicer to program against for extensibility and testing. Though in this case > > it looks like Window is ABC not an interface. Window is an ABC, and is intended to be used just by internal and implementation classes. I will add a public interface class as a return class. Thanks for the suggestion. > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java, > > line 44 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498259#file1498259line44> > > > > You can use a private constructor to prevent subclassing / > > instantiation. Make sense! Will do. The same for MessageStreams. > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java, > > line 332 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498259#file1498259line332> > > > > Can you drop input here as it is actually unused? I found my way here > > due to seeing something else handing this the "this" pointer, which looked > > suspicious. Turns out it is not used - maybe for type parameter bounds > > checking? but is so, I'd be interested to see if we could find a more > > direct and obvious approach. > > > > --- > > > > Also, as far as I can tell, this looks like it can be moved straight > > into MessageStream (perhaps as a private method). I didn't see anything > > else using it, but maybe I missed it. As we discussed, I will drop it for now and revisit it in the later patch that implements the execution graph. > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/task/StreamOperatorTask.java, > > line 39 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498269#file1498269line39> > > > > Minor: abstract not necessary here. (Technically public isn't either > > :)). Yeah, it is a copy/paste problem when move this method to a separate interface class. Thanks for pointing out! > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/test/java/org/apache/samza/operators/api/data/TestMessageStream.java, > > lines 42-61 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498270#file1498270line42> > > > > This doesn't appear to be testing anything (other than that types > > check). This RB is limited to APIs. I will add more assert/test code to ensure the internal variable/methods are setup/invoked correctly. > On Sept. 14, 2016, 7:03 p.m., Chris Pettitt wrote: > > samza-operator/src/test/java/org/apache/samza/task/AssembleCallGraphTask.java, > > line 141 > > <https://reviews.apache.org/r/47835/diff/15/?file=1498271#file1498271line141> > > > > Minor: indentation is off here. Sure. - Yi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47835/#review148908 ----------------------------------------------------------- On Sept. 14, 2016, 8:53 a.m., Yi Pan (Data Infrastructure) wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47835/ > ----------------------------------------------------------- > > (Updated Sept. 14, 2016, 8:53 a.m.) > > > Review request for samza, Boris Shkolnik, Chris Pettitt, Chinmay Soman, Jake > Maes, Navina Ramesh, Jagadish Venkatraman, and Xinyu Liu. > > > Bugs: SAMZA-914 > https://issues.apache.org/jira/browse/SAMZA-914 > > > Repository: samza > > > Description > ------- > > SAMZA-914: initial draft of operator programming API. Design doc attached to > SAMZA-914: > https://issues.apache.org/jira/secure/attachment/12821524/SAMZA-914_%20operator%20Java%20programming%20API%20-%20Google%20Docs.pdf > > > Diffs > ----- > > build.gradle 16facbbf4dff378c561461786ff186bd9e0000ed > gradle/dependency-versions.gradle 52e25aa53a1edc85d478b48898621b26508ad4bb > samza-api/src/test/java/org/apache/samza/config/TestConfig.java > 5d066c5867e9df9e94e60bde825dedf10703b399 > > samza-operator/src/main/java/org/apache/samza/operators/api/MessageStream.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/MessageStreams.java > PRE-CREATION > samza-operator/src/main/java/org/apache/samza/operators/api/Triggers.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/WindowState.java > PRE-CREATION > samza-operator/src/main/java/org/apache/samza/operators/api/Windows.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/data/Message.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/data/WindowOutput.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/api/internal/Trigger.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/internal/Window.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/OperatorBaseImpl.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/ProcessorContext.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/SimpleOperatorImpl.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/StateStoreImpl.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/impl/window/SessionWindowImpl.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/task/StreamOperatorAdaptorTask.java > PRE-CREATION > samza-operator/src/main/java/org/apache/samza/task/StreamOperatorTask.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/api/data/TestMessageStream.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/task/AssembleCallGraphTask.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/InputAvroSystemMessage.java > PRE-CREATION > samza-operator/src/test/java/org/apache/samza/task/JoinOperatorTask.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/task/TestStreamOperatorTasks.java > PRE-CREATION > samza-operator/src/test/java/org/apache/samza/task/WindowOperatorTask.java > PRE-CREATION > > samza-sql-calcite/src/test/java/org/apache/samza/sql/calcite/schema/TestAvroSchemaConverter.java > PRE-CREATION > samza-sql-core/README.md PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/api/data/Data.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/api/data/EntityName.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/api/data/Relation.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/api/data/Schema.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/api/data/Stream.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/api/data/Table.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/api/data/Tuple.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/api/operators/Operator.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/api/operators/OperatorCallback.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/api/operators/OperatorRouter.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/api/operators/OperatorSpec.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/api/operators/SimpleOperator.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/api/operators/SqlOperatorFactory.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/data/IncomingMessageTuple.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/data/avro/AvroData.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/sql/data/avro/AvroSchema.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/data/serializers/SqlAvroSerde.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/data/serializers/SqlAvroSerdeFactory.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/data/serializers/SqlStringSerde.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/data/serializers/SqlStringSerdeFactory.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/data/string/StringData.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/data/string/StringSchema.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/factory/NoopOperatorCallback.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/factory/SimpleOperatorFactoryImpl.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/factory/SimpleOperatorImpl.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/factory/SimpleOperatorSpec.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/factory/SimpleRouter.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/join/StreamStreamJoin.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/join/StreamStreamJoinSpec.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/partition/PartitionOp.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/partition/PartitionSpec.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/window/BoundedTimeWindow.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/window/WindowSpec.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/operators/window/WindowState.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/sql/window/storage/OrderedStoreKey.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/system/sql/LongOffset.java > PRE-CREATION > samza-sql-core/src/main/java/org/apache/samza/system/sql/Offset.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/task/sql/RouterMessageCollector.java > PRE-CREATION > > samza-sql-core/src/main/java/org/apache/samza/task/sql/SimpleMessageCollector.java > PRE-CREATION > > samza-sql-core/src/test/java/org/apache/samza/sql/data/serializers/SqlAvroSerdeTest.java > PRE-CREATION > > samza-sql-core/src/test/java/org/apache/samza/task/sql/RandomWindowOperatorTask.java > PRE-CREATION > samza-sql-core/src/test/java/org/apache/samza/task/sql/StreamSqlTask.java > PRE-CREATION > > samza-sql-core/src/test/java/org/apache/samza/task/sql/UserCallbacksSqlTask.java > PRE-CREATION > settings.gradle 4c1aa107a11d413777e69bc4e48847b811aff7d2 > > Diff: https://reviews.apache.org/r/47835/diff/ > > > Testing > ------- > > ./gradlew clean build > > > Thanks, > > Yi Pan (Data Infrastructure) > >
