samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jake Maes <jma...@apache.org>
Subject Re: Review Request 47994: SAMZA-915: implementation of StreamPipeline and operator runtime impl classes
Date Fri, 07 Oct 2016 21:04:16 GMT

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


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