Till Westmann has posted comments on this change. Change subject: Add Asterix Extension Manager ......................................................................
Patch Set 7: (7 comments) Only a partial review for now as I won't finish tonight. I like the reduction of code in the metadata manager and the associated gain in extensibility. I'm a little confused by all the formatting errors as I thought that the formatter will take care of those ... https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/AInt32.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/base/AInt32.java: Line 26: import org.json.JSONObject; Revert this file? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IConnectorDescriptor.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IConnectorDescriptor.java: Line 146: public ConnectorDescriptorId copy(IConnectorDescriptorRegistry registry); Shouldn't this be a "Factory" instead of a "Registry" id all it does is to manufacture ids (probably the name of the method should be changed as well - very confusing ...). Maybe filing an issue for this is the best approach. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/IOperatorDescriptor.java: Line 47: void setOperatorId(OperatorDescriptorId id); I think that we should try to avoid the modification of this interface. 1) Changing the interface will force all other implementers of this interface to change. 2) The id should really be given at construction time. 3) There should be no setter on this interface (removing setDisplayName seems to be safe as it seems to be unused ...). https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/job/JobSpecification.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/job/JobSpecification.java: Line 101: new HashMap<ConnectorDescriptorId, Pair<Pair<IOperatorDescriptor, Integer>, Pair<IOperatorDescriptor, Integer>>>(); 2 birds with 1 stone! https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractSingleActivityOperatorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/base/AbstractSingleActivityOperatorDescriptor.java: Line 45: if (activityNodeId != null && !activityNodeId.getOperatorDescriptorId().equals(odId)) { Why would the activityNodeId be null? It is final and set in the constructor. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/LocalityAwareMToNPartitioningConnectorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/LocalityAwareMToNPartitioningConnectorDescriptor.java: Line 94: public ConnectorDescriptorId copy(IConnectorDescriptorRegistry spec) { Why is an IConnectorDescriptorRegistry called "spec"? And what does "copy" do with the "spec"? https://asterix-gerrit.ics.uci.edu/#/c/1017/7/hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/OneToOneConnectorDescriptor.java File hyracks-fullstack/hyracks/hyracks-dataflow-std/src/main/java/org/apache/hyracks/dataflow/std/connectors/OneToOneConnectorDescriptor.java: Line 76: } catch (Throwable th) { This seems bad. We should do it right, remove it, or file an issue for it. -- To view, visit https://asterix-gerrit.ics.uci.edu/1017 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I280268495cc3aad00f898cba21f7299f7120ce5c Gerrit-PatchSet: 7 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Steven Jacobs <sjaco...@ucr.edu> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Yingyi Bu <buyin...@gmail.com> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
