samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Yi Pan \(Data Infrastructure\)" <yi...@linkedin.com>
Subject Re: Review Request 47994: SAMZA-915: implementation of StreamPipeline and operator runtime impl classes
Date Wed, 05 Oct 2016 07:50:14 GMT


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


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