Till Westmann has posted comments on this change. Change subject: Runtime ErrorCode fix in external data ......................................................................
Patch Set 5: (11 comments) - There are a number of places (interfaces, catch clauses) where "Exception" is replaced by a more specific checked exception. This might change the system behavior as "Exception" also catches unchecked RuntimeExceptions. Did you carefully consider this possibility in each of those cases? - The same is care is needed for changes that wrap all Exceptions/Throwables in checked exceptions. - When modifying the throws clauses on interface method it is especially important to consider a) the consistent use of exceptions across interface methods and b) the impact of the change on existing implementations of the the interface that might not be in this code base. - Please address the SQ comments. https://asterix-gerrit.ics.uci.edu/#/c/1374/5/asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml File asterixdb/asterix-app/src/test/resources/runtimets/testsuite.xml: Line 6559: <expected-error>a quote enclosing a field needs to be placed in the beginning of that field.</expected-error> Start message with a capital a? Line 6565: <expected-error>a quote enclosing a field needs to be placed in the beginning of that field.</expected-error> Start message with a capital a? https://asterix-gerrit.ics.uci.edu/#/c/1374/5/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: Line 55: } catch (HyracksDataException ex) { Is it indeed the intention to handle only HyracksDataExceptions and no RuntimeExceptions? The previous version of this code also handled (potentially undeclared) RuntimeExceptions. Line 66: } catch (HyracksDataException exception) { Is it indeed the intention to handle only HyracksDataExceptions and no RuntimeExceptions? The previous version of this code also handled (potentially undeclared) RuntimeExceptions. https://asterix-gerrit.ics.uci.edu/#/c/1374/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameSpiller.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameSpiller.java: Line 130: } catch (IOException e) { Is it indeed the intention to handle only IOException and no RuntimeExceptions? The previous version of this code also handled (potentially undeclared) RuntimeExceptions. https://asterix-gerrit.ics.uci.edu/#/c/1374/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/ActiveLifecycleEventSubscriber.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/management/ActiveLifecycleEventSubscriber.java: Line 57: throw new HyracksDataException(e1); Will this surely end the thread? https://asterix-gerrit.ics.uci.edu/#/c/1374/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/AdapterExecutor.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/AdapterExecutor.java: Line 59: } catch (HyracksDataException e) { Is it indeed the intention to handle only HyracksDataExceptions and no RuntimeExceptions? The previous version of this code also handled (potentially undeclared) RuntimeExceptions. https://asterix-gerrit.ics.uci.edu/#/c/1374/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/AdapterRuntimeManager.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/AdapterRuntimeManager.java: Line 80: } catch (ExecutionException exception) { Is it indeed the intention to handle only ExecutionException and no RuntimeExceptions? The previous version of this code also handled (potentially undeclared) RuntimeExceptions. https://asterix-gerrit.ics.uci.edu/#/c/1374/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/CollectionRuntime.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/CollectionRuntime.java: Line 64: throw new HyracksDataException(e); Will this surely exit the thread? https://asterix-gerrit.ics.uci.edu/#/c/1374/5/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/DelimitedDataParser.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/DelimitedDataParser.java: Line 117: private void parseRecord(DataOutput out) throws HyracksDataException { > MAJOR SonarQube violation: That seems to be a good point ... https://asterix-gerrit.ics.uci.edu/#/c/1374/5/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/base/IClusterController.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-common/src/main/java/org/apache/hyracks/control/common/base/IClusterController.java: Line 65: HyracksDataException; I think that we should not change this interface. All other methods throw a plain Exception, so changing this one doesn't really help. And it potentially breaks other implementations of this interface. -- To view, visit https://asterix-gerrit.ics.uci.edu/1374 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ida810a56bf4aef1394879f088a6a5e8f82c60b74 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-HasComments: Yes
