abdullah alamoudi has posted comments on this change. Change subject: Ensure nextFrame and flush are not called in a failed pipeline ......................................................................
Patch Set 13: (19 comments) https://asterix-gerrit.ics.uci.edu/#/c/1618/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/FeedOperations.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/utils/FeedOperations.java: Line 287: for (Entry<ConnectorDescriptorId, Pair<Pair<IOperatorDescriptor, Integer>, Pair<IOperatorDescriptor, Integer>>> entry : subJob > +1 Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedExceptionHandler.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedExceptionHandler.java: PS13, Line 60: if (LOGGER.isLoggable(Level.WARNING)) { > remove Done PS13, Line 61: " + ex.getMessage() > remove appending .getMessage(), pass exception to logger call Done PS13, Line 70: exception.printStackTrace(); > remove Done PS13, Line 71: if (LOGGER.isLoggable(Level.WARNING)) { > remove Done PS13, Line 72: " + exception.getMessage() > remove appending .getMessage(), pass exception to logger call Done PS13, Line 80: e.getMessage() > this should be a descriptive string, and I would guess include the tupleInd Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/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: Line 74: IFrameWriter writer, FeedPolicyAccessor fpa, FrameTupleAccessor fta, ConcurrentFramePool framePool) > MAJOR SonarQube violation: Done Line 74: IFrameWriter writer, FeedPolicyAccessor fpa, FrameTupleAccessor fta, ConcurrentFramePool framePool) > should we remove this parameter if it is not needed? Done PS13, Line 377: } catch (Throwable th) { : this.cause = th; : return th; > It seems much cleaner to just let this exception propagate- not clear why w Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/AbstractFrameWriter.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/AbstractFrameWriter.java: PS13, Line 29: AbstractFrameWriter > Could we add a comment here describing the division of labor between the Ab Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/IExceptionHandler.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/IExceptionHandler.java: Line 38: * @return returns a new frame with tuples excluding the exception generating tuple or null if exception was not handled > +1 Done PS13, Line 39: * @throws HyracksDataException > doesn't seem to Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/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: Line 187: (predEvaluatorFactory == null ? null : predEvaluatorFactory.createPredicateEvaluator()); > +1 Done Line 380: (predEvaluatorFactory == null ? null : predEvaluatorFactory.createPredicateEvaluator()); > +1 Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/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: Line 272: (predEvaluatorFactory == null ? null : predEvaluatorFactory.createPredicateEvaluator()); > +1 Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/AbstractSortRunGenerator.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/sort/AbstractSortRunGenerator.java: PS13, Line 68: flushWriter.fail(); > this will lose the original exception in case of failure in fail()- would b Done https://asterix-gerrit.ics.uci.edu/#/c/1618/13/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexBulkLoadOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexBulkLoadOperatorNodePushable.java: PS13, Line 108: throw new HyracksDataException(th); > this loses interrupted exceptions Interrupted Exception is a declared exception. bulkLoader.end() only throws HDE or IndexException PS13, Line 118: throw th; > this will lose the original exception Done -- 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: 13 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
