Till Westmann has posted comments on this change. Change subject: WIP ......................................................................
Patch Set 13: (12 comments) https://asterix-gerrit.ics.uci.edu/#/c/1875/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/IStatementExecutor.java: PS13, Line 169: /** : * @return the storage component provider : */ : IStorageComponentProvider getComponentProvider(); : : /** : * @return the application context : */ : ICcApplicationContext getApplicationContext(); This seems to be the wrong interface for these methods. Why would a statement executor expose the storage component provider or the application context? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/GlobalRecoveryManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/GlobalRecoveryManager.java: PS13, Line 58: protected ClusterState state; It seems that the state is not needed anymore. Is that right? If so, should we remove it? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IClusterEventsSubscriber.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IClusterEventsSubscriber.java: PS13, Line 58: refreshState Why rename this? Everything in this interface is about cluster state notifications ... the new name is confusing. https://asterix-gerrit.ics.uci.edu/#/c/1875/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/cluster/IClusterStateManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/cluster/IClusterStateManager.java: PS13, Line 118: setGlobalRecoveryCompleted Just looking at the names here it seems that this should be on the IGlobalRecoveryManager and not on the IClusterStateManager. https://asterix-gerrit.ics.uci.edu/#/c/1875/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/cluster/IGlobalRecoveryManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/cluster/IGlobalRecoveryManager.java: PS13, Line 30: HyracksDataException Document when this newly added exception is raised? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/ICcApplicationContext.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/dataflow/ICcApplicationContext.java: PS13, Line 70: getActiveNotificationHandler Why this difference between the return type and the method name? PS13, Line 99: MetadataLockManager Should we have an interface here? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/exceptions/ErrorCode.java: PS13, Line 117: REMOTING_EXCEPTION_WHEN_CALLING_METADATA_NODE s/REMOTING/REMOTE/? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksDataException.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksDataException.java: PS13, Line 56: What was wrong with this approach? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksException.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/exceptions/HyracksException.java: PS13, Line 33: protected I agree with SQ here, we should keep exceptions unmodifiable. What was wrong with the previous approach? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/application/CCServiceContext.java File hyracks-fullstack/hyracks/hyracks-control/hyracks-control-cc/src/main/java/org/apache/hyracks/control/cc/application/CCServiceContext.java: PS13, Line 92: , JobStatus jobStatus, List<Exception> exceptions Can't we get this information from Hyracks' JobManager? https://asterix-gerrit.ics.uci.edu/#/c/1875/13/hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/tuples/TypeAwareTupleWriter.java File hyracks-fullstack/hyracks/hyracks-storage-am-common/src/main/java/org/apache/hyracks/storage/am/common/tuples/TypeAwareTupleWriter.java: Line 38: System.err.println("huh!"); > MAJOR SonarQube violation: Agreed :) -- To view, visit https://asterix-gerrit.ics.uci.edu/1875 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifeac8c73e6bad39a13663b84a52121356e3c6b40 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: Murtadha Hubail <mhub...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com> Gerrit-HasComments: Yes
