----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47835/#review148908 -----------------------------------------------------------
This review turned out not to be so massive as I expected (lots of deletes). I don't see any serious issues. There are some minor cosmetic issues and some subjective stuff that you can take or leave. I tried to not mark subjective stuff as an issue. samza-operator/src/main/java/org/apache/samza/operators/api/MessageStream.java (line 53) <https://reviews.apache.org/r/47835/#comment216420> 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. samza-operator/src/main/java/org/apache/samza/operators/api/MessageStream.java (line 67) <https://reviews.apache.org/r/47835/#comment216415> 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. samza-operator/src/main/java/org/apache/samza/operators/api/MessageStreams.java (line 32) <https://reviews.apache.org/r/47835/#comment216424> Private constructor. I explain the benefits either above or below (been jumping around a bit). samza-operator/src/main/java/org/apache/samza/operators/api/Triggers.java (line 30) <https://reviews.apache.org/r/47835/#comment216431> It's probably worth noting that if there are multiple triggers the aggregate value is the disjunction of the individual values. samza-operator/src/main/java/org/apache/samza/operators/api/Triggers.java (line 39) <https://reviews.apache.org/r/47835/#comment216429> Maybe TriggerBuilder as this differs a bit from the typical class of static factory methods implied by the class name? samza-operator/src/main/java/org/apache/samza/operators/api/Windows.java (line 48) <https://reviews.apache.org/r/47835/#comment216485> 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. samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java (line 44) <https://reviews.apache.org/r/47835/#comment216417> You can use a private constructor to prevent subclassing / instantiation. samza-operator/src/main/java/org/apache/samza/operators/api/internal/Operators.java (line 332) <https://reviews.apache.org/r/47835/#comment216418> 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. samza-operator/src/main/java/org/apache/samza/operators/api/internal/Window.java (line 39) <https://reviews.apache.org/r/47835/#comment216486> IIUC given this interface Window is basically an entirely opaque object to the client code? samza-operator/src/main/java/org/apache/samza/task/StreamOperatorTask.java (line 39) <https://reviews.apache.org/r/47835/#comment216487> Minor: abstract not necessary here. (Technically public isn't either :)). samza-operator/src/test/java/org/apache/samza/operators/api/data/TestMessageStream.java (lines 42 - 61) <https://reviews.apache.org/r/47835/#comment216488> This doesn't appear to be testing anything (other than that types check). samza-operator/src/test/java/org/apache/samza/task/AssembleCallGraphTask.java (line 141) <https://reviews.apache.org/r/47835/#comment216489> Minor: indentation is off here. - Chris Pettitt 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) > >
