Yingyi Bu has posted comments on this change. Change subject: Ensure nextFrame and flush are not called in a failed pipeline ......................................................................
Patch Set 5: (10 comments) https://asterix-gerrit.ics.uci.edu/#/c/1618/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java: PS5, Line 95: synchronized why synchronized is needed? writer is shared among partitions? https://asterix-gerrit.ics.uci.edu/#/c/1618/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/SyncFeedRuntimeInputHandler.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/SyncFeedRuntimeInputHandler.java: PS5, Line 54: unfail Couldn't understand why you need to call writer.unfail() and it's only for an AbstractUnaryInputUnaryOutputOperatorNodePushable instance. https://asterix-gerrit.ics.uci.edu/#/c/1618/5/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractUnaryInputUnaryOutputOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractUnaryInputUnaryOutputOperatorNodePushable.java: PS5, Line 44: Override The state machine enforcement is nice. But I think it should be moved to IFrameWriter, instead of AbstractUnaryInputUnaryOutputOperatorNodePushable. Why AbstractUnaryInputUnaryOutputOperatorNodePushable is special in this regard? PS5, Line 56: Log the stack trace so that we know which operator does not do the right thing. PS5, Line 69: LOGGER Log the stack trace so that we know which operator does not do the right thing. PS5, Line 79: unfail This seems error-prone. Once an IFrameWriter failed, it should be failed forever, no? https://asterix-gerrit.ics.uci.edu/#/c/1618/5/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/HybridHashJoinOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/HybridHashJoinOperatorDescriptor.java: PS5, Line 577: The probe branch could be flushed since you might have sth. in the output buffer, e.g., in case the probe data hits in-memory partitions? https://asterix-gerrit.ics.uci.edu/#/c/1618/5/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/InMemoryHashJoinOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/InMemoryHashJoinOperatorDescriptor.java: PS5, Line 295: The probe branch could be flushed since you might have sth. in the output buffer since the probe is in-memory? https://asterix-gerrit.ics.uci.edu/#/c/1618/5/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/NestedLoopJoinOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/NestedLoopJoinOperatorDescriptor.java: PS5, Line 223: No The probe branch could be flushed since you might have sth. in the output buffer? https://asterix-gerrit.ics.uci.edu/#/c/1618/5/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/OptimizedHybridHashJoinOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/join/OptimizedHybridHashJoinOperatorDescriptor.java: PS5, Line 820: } The probe branch could be flushed since you might have sth. in the output buffer, e.g., in case the probe data hits in-memory partitions? -- To view, visit https://asterix-gerrit.ics.uci.edu/1618 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9827b06f640858f27ec1bcca2a39991780bee3b1 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
