> On Aug. 26, 2016, 7:34 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/window/Trigger.java, > > line 28 > > <https://reviews.apache.org/r/47835/diff/12/?file=1481733#file1481733line28> > > > > Definitely not digging the mutable state here. > > > > Trigger a = Trigger.of(...); > > Trigger b = Trigger.of(...); > > Trigger c = Trigger.of(...); > > > > Trigger ab = a.and(b); > > Trigger ac = a.and(c); > > > > Now both ab and ac are really a and b and c. Imagine if this weren't > > all in one function! > > > > It appears that Trigger may not actually be necessary. Essentially this > > class is just a set of helpers for building predicates. I could imagine > > something like this: > > > > ``` > > public static class PredicateHelper { > > public static <S> Function<S, Boolean> and(Function<S, Boolean> > > lhs, Function<S, Boolean> rhs) { > > return new Function<S, Boolean>() { > > @Override > > public Boolean apply(S s) { > > return lhs.apply(s) && rhs.apply(s); > > } > > }; > > } > > } > > ``` > > > > Main point though is to use immutability.
You are completely right that this is a way to build predicates. I will try this out w/ the new Trigger API updates. > On Aug. 26, 2016, 7:34 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/window/Window.java, > > line 122 > > <https://reviews.apache.org/r/47835/diff/12/?file=1481734#file1481734line122> > > > > Minor: we should be consistent with camel case. Yep, fixed in the updated patch. > On Aug. 26, 2016, 7:34 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/window/Window.java, > > line 41 > > <https://reviews.apache.org/r/47835/diff/12/?file=1481734#file1481734line41> > > > > Last time I'll beat on this dead horse. Definitely favor immutability > > where you can. Left to the reader to determine whether that is desirable. Are you referring to all the member variables being final when mentioning "immutability"? The problem I have w/ is that the user will now need to specify all variables in constructor. W/ all possible combination of user choices in whether include/use a certain function or not, the versions of constructor would explode. For example, when a user choose to specify watermark or lateArrival optionally, there will need to be different versions of constructor for this class to support the immutability. Also, there has been a use case in Calcite integration we had before, that the query planner/optimizer does not know the actual parameters for the operator yet while creating the operator instance. The desire was to use a builder pattern to gradually build-up the operator. Let's sync up offline. > On Aug. 26, 2016, 7:34 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/task/StreamOperatorTask.java, > > lines 45-59 > > <https://reviews.apache.org/r/47835/diff/12/?file=1481741#file1481741line45> > > > > If this is all the user really needs to implement could we do this as > > an interface instead? Let me try this solution. One question that I have is: there is some logic that I wanted to enforce regarding to when to call the user implemented initOnce() and initPipeline() methods in the StreamOperatorTask.init(). How do I enforece it if I convert StreamOperatorTask to an interface class? > On Aug. 26, 2016, 7:34 p.m., Chris Pettitt wrote: > > samza-operator/src/main/java/org/apache/samza/operators/api/window/Window.java, > > line 131 > > <https://reviews.apache.org/r/47835/diff/12/?file=1481734#file1481734line131> > > > > If the only reason to make this abstract is the function can we just > > take it as an arg and about class hierarchy? I can remove it for now, since there is only one implementation of Window class. This might be needed later in the window implementation classes, when we have more windows implemented. I do have one thing that can't find a solution for yet: these functions are actually only accessed within the implementation classes in Samza, should not be accessed by the programmer. How do I hide them while still provide external access to those method in org.apache.samza.operator.impl package? The only potential solution I can think of is to fold these methods into package private and expose to implementation classes via WindowOperator inside MessageStream. However, I will have to move Window related classes to the same package path w/ MessageStream, which makes the source file structure less modular. I would be appreciate if you have any suggestion on this! Thanks! - Yi ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47835/#review147014 ----------------------------------------------------------- On Aug. 26, 2016, 8:43 p.m., Yi Pan (Data Infrastructure) wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47835/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 8:43 p.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/Scan.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/data/window/WindowOutput.java > PRE-CREATION > samza-operator/src/main/java/org/apache/samza/operators/api/join/Join.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/window/SessionWindow.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/window/Trigger.java > PRE-CREATION > > samza-operator/src/main/java/org/apache/samza/operators/api/window/Window.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/Pipeline.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/SinkOperatorImpl.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/StreamOperatorTask.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/api/TestScanner.java > PRE-CREATION > > samza-operator/src/test/java/org/apache/samza/operators/api/data/TestDataStream.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/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) > >
