asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Till Westmann (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: WIP
Date Sun, 23 Jul 2017 18:10:48 GMT
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 <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Murtadha Hubail <mhubail@apache.org>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Xikui Wang <xkkwww@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message