abdullah alamoudi 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 stateme Do you prefer passing two additional parameters when creating active listeners? 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 Done 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 notifi Thought the renaming comes with the signature change since we don't pass the previous state. Done 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 IGlobalR Done 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? Done 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? dependency level. the IActiveNotificationHandler is at a higher level than ICcApplicationContext PS13, Line 99: MetadataLockManager > Should we have an interface here? Done 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/? Done 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? This is not right and can screw up the error message. For example, you have a n ASX error message, this creates a HyracksDataException which will look for the error message in the Hyracks Error Codes file. and passing e.getMessage() to the constructor is meaningless. 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 wron What was wrong with the previous way was explained in the other file. The right way seems to be to set the nodeId at the time of creating the first exception... but at least, this way will not screw up the error message 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? I don't understand. Can you clarify? 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!"); > Agreed :) Done -- 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-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
