Michael Blow has posted comments on this change. Change subject: Ensure nextFrame and flush are not called in a failed pipeline ......................................................................
Patch Set 13: (20 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 > MAJOR SonarQube violation: +1 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 PS13, Line 61: " + ex.getMessage() remove appending .getMessage(), pass exception to logger call PS13, Line 70: exception.printStackTrace(); remove PS13, Line 71: if (LOGGER.isLoggable(Level.WARNING)) { remove PS13, Line 72: " + exception.getMessage() remove appending .getMessage(), pass exception to logger call PS13, Line 80: e.getMessage() this should be a descriptive string, and I would guess include the tupleIndex. Throwable.getMessage() is not useful for most exceptions. 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: should we remove this parameter if it is not needed? PS13, Line 377: } catch (Throwable th) { : this.cause = th; : return th; It seems much cleaner to just let this exception propagate- not clear why we are catching here and using a return value to signal failure https://asterix-gerrit.ics.uci.edu/#/c/1618/13/hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/AlgebricksMetaOperatorDescriptor.java File hyracks-fullstack/algebricks/algebricks-runtime/src/main/java/org/apache/hyracks/algebricks/runtime/operators/meta/AlgebricksMetaOperatorDescriptor.java: PS13, Line 130: opened = true; we want to call close() and fail() on startOfPipeline, even if open fails? 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 > MAJOR SonarQube violation: +1 PS13, Line 39: * @throws HyracksDataException doesn't seem to 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()); > MAJOR SonarQube violation: +1 Line 380: (predEvaluatorFactory == null ? null : predEvaluatorFactory.createPredicateEvaluator()); > MAJOR SonarQube violation: +1 https://asterix-gerrit.ics.uci.edu/#/c/1618/13/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: Line 188: (predEvaluatorFactory == null ? null : predEvaluatorFactory.createPredicateEvaluator()); > MAJOR SonarQube violation: +1 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()); > MAJOR SonarQube violation: +1 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 be bad in case of interrupted 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 PS13, Line 118: throw th; this will lose the original exception 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/IndexSearchOperatorNodePushable.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/dataflow/IndexSearchOperatorNodePushable.java: PS13, Line 215: closeException = new HyracksDataException(th); this loses interrupted exceptions -- 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 <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
