samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Pettitt <cpett...@linkedin.com>
Subject Re: Review Request 47835: SAMZA-914: Initial draft for Java programming APIs on operators supporting DAGs
Date Wed, 14 Sep 2016 19:03:48 GMT

-----------------------------------------------------------
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)
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message