abdullah alamoudi has posted comments on this change. Change subject: Add Asterix Extension Manager ......................................................................
Patch Set 7: (22 comments) https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/AlgebraUtils.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/AlgebraUtils.java: Line 31: public class AlgebraUtils { > 1. Rename: "AlgebraUtils" -> "ConstantExpressionUtil" Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/APIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/APIServlet.java: Line 70: ILangCompilationProvider sqlppCompilationProvider, IStatementExecutorFactory queryTranslatorFactory) { > "queryTranslatorFactory" -> "statementExecutorFactory" Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/AQLAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/AQLAPIServlet.java: Line 35: public AQLAPIServlet(ILangCompilationProvider compilationProvider, IStatementExecutorFactory queryTranslatorFactory) { > variable name doesn't seem to match the type: Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/DDLAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/DDLAPIServlet.java: Line 43: public DDLAPIServlet(ILangCompilationProvider compilationProvider, IStatementExecutorFactory queryTranslatorFactory) { > "queryTranslatorFactory"-> "statementExecutorFactory" Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/java/AsterixJavaClient.java: Line 65: ILangCompilationProvider compilationProvider, IStatementExecutorFactory queryTranslatorFactory) { > queryTranslatorFactory -> statementExecutorFactory Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/ICCExtensionManager.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/extension/ICCExtensionManager.java: Line 21: public interface ICCExtensionManager { > I guess the interface should have sth... Otherwise its role can be played Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/pom.xml File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/pom.xml: Line 36: <version>2.0.8</version> > Use 3.4 as well, like the above one? doesn't exist for artifact maven-project. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/lang/extension.jj File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/lang/extension.jj: Line 8: // As a workaround, you can always override > >> refrain from using the strings "before:" and "after:". before means that the text will appear before the base text in the original class in this block. after means that the text will appear after the base text block. Line 9: // one additional possible change is direct replacement and it can be done through @merge replace existing with new > 1. Does the sentence complete? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/unit/basic-test/basic-test-plugin-config.xml File asterixdb/asterix-maven-plugins/asterix-grammar-extension-maven-plugin/src/test/resources/unit/basic-test/basic-test-plugin-config.xml: Line 29: <version>2.0.6</version> > Using the maven version consistent to other places? e.g., 3.4? For maven-project artifact, 3.0-alpha-2 is the latest and It breaks many things in our build https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/MetadataNode.java: Line 135: private Map<ExtensionMetadataDatasetId, ExtensionMetadataDataset<?>> extensionIndexes; > Why is variable called "...Indexes"? Call it "....Map"? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ExtensionMetadataDatasetId.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ExtensionMetadataDatasetId.java: Line 31: public ExtensionMetadataDatasetId(ExtensionId extensionId, String indexName) { > indexName -> datasetName? Done Line 40: public String getIndexName() { > IndexName -> DatasetName? Done Line 48: return extensionId.equals(otherId.getExtensionId()) && indexName.equals(otherId.getIndexName()); > Use org.apache.commons.lang3.ObjectUtils.equals(...) to make it simple. Done Line 55: return indexName.hashCode(); > Why not also use extendId here? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ICachableMetadataEntity.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/ICachableMetadataEntity.java: Line 24: public interface ICachableMetadataEntity<T> extends IMetadataEntity { > Why do we need the "Cachable" prefix? Is there any MetadataEntity that is n There are many that are not cachable. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IExtensionMetadataEntity.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IExtensionMetadataEntity.java: Line 23: public interface IExtensionMetadataEntity extends IMetadataEntity { > Document the interface and methods Done Line 25: ExtensionMetadataDatasetId getIndexId(); > Why does every extension metadata entity have an index? This should've been called getDatasetId. Since we don't know the extension entity should go to which metadata dataset, we need a way to get that information. https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataExtension.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/IMetadataExtension.java: Line 29: /** > A new line between the last import and comments. (Or, format the source fil Done https://asterix-gerrit.ics.uci.edu/#/c/1017/7/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/MetadataTupleTranslatorProvider.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entitytupletranslators/MetadataTupleTranslatorProvider.java: Line 26: public class MetadataTupleTranslatorProvider implements Serializable{ > Does this Provider need to transfer across nodes? Done 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. Let's discuss this? I need this in order to be able to use operators of a created job in a new job.x 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 constructo There are cases where it is null. It can be called from the constructor in super() before it gets an assigned value. -- 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
