abdullah alamoudi has posted comments on this change. Change subject: Add Asterix Extension Manager ......................................................................
Patch Set 6: (36 comments) https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/base/AbstractLangExtension.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/base/AbstractLangExtension.java: Line 25: public abstract class AbstractLangExtension implements IExtension { > Should we make this an interface? There's not much shared functionality her Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IAccessMethod.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/IAccessMethod.java: Line 40: public interface IAccessMethod extends Comparable<IAccessMethod> { > Can we revert the file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ConnectorAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/ConnectorAPIServlet.java: Line 22: import static org.apache.asterix.api.http.servlet.ServletConstants.HYRACKS_CONNECTION_ATTR; > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/AbstractQueryTranslatorExtension.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/AbstractQueryTranslatorExtension.java: Line 24: public abstract class AbstractQueryTranslatorExtension implements IExtension { > Should we make this an interface? There's not much shared functionality her Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/AbstractStatementHandler.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/AbstractStatementHandler.java: Line 33: public abstract void handle(AqlMetadataProvider metadataProvider, IExtensionStatement extensionStatement, > The metadata provider should really not contain AQL - but that's not for th Totally agree. I think it should be made a change by itself since it will touch so many files. https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CCExtensionManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CCExtensionManager.java: Line 52: * @param extensions > That's not the name of the parameter ... Done Line 68: throw new IllegalArgumentException("Two Extensions share the same Id " + extension.getId()); > I think that we don't need to throw RuntimeExceptions in this constructor. Done Line 111: IHyracksClientConnection hcc) throws HyracksDataException { > It seems that we could just add a "handle" or "process" method to the IExte I think that's a terrific idea. Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/AsterixNCAppRuntimeContext.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/AsterixNCAppRuntimeContext.java: Line 141: this.metadataRmiPort = metadataRmiPort; > We should leave this wehre it was - that's closer to the declaration order. Sure. I just moved it up there since it is a simple address assignment https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/DefaultQueryTranslatorFactory.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/DefaultQueryTranslatorFactory.java: Line 30: private final CCExtensionManager ccExtensionManager; > Should this be an ICCExtensionManager, if we have that interface? Or should Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/IQueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/IQueryTranslator.java: Line 26: public interface IQueryTranslator { > This should really be an IStatementExecutor ... Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/IQueryTranslatorFactory.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/IQueryTranslatorFactory.java: Line 31: ILangCompilationProvider compliationProvider); > s/compliationProvider/compilationProvider/ Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/translator/QueryTranslator.java: Line 225: protected final CCExtensionManager ccExtensionManager; > Should this be an ICCExtensionManager, if we have that interface? Or should I think this should be a concrete class https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/file/DatasetOperations.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/file/DatasetOperations.java: Line 96: MetadataManager.INSTANCE.getDataverse(metadataProvider.getMetadataTxnContext(), dataverseName); > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/file/DataverseOperations.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/file/DataverseOperations.java: Line 34: metadata.splitProviderAndPartitionConstraintsForDataverse(dataverse.getDataverseName()); > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/CCApplicationEntryPoint.java: Line 81: protected CCExtensionManager ccExtensionManager; > Should this be an ICCExtensionManager, if we have that interface? Or should I think let's keep it for now and then decide later if we want to change it? https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/result/ResultUtils.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/result/ResultUtils.java: Line 27: import java.util.HashMap; > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/util/FlushDatasetUtils.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/util/FlushDatasetUtils.java: Line 47: throws Exception { > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/test/java/org/apache/asterix/api/http/servlet/ConnectorAPIServletTest.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/api/http/servlet/ConnectorAPIServletTest.java: Line 100: new JSONTokener(new InputStreamReader(new ByteArrayInputStream(outputStream.toByteArray()))); > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IDataSourceAdapter.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IDataSourceAdapter.java: Line 19: package org.apache.asterix.common.api; > Why does this need to move to common? Isn't external the right package? this was moved when I was trying to do the extensions the wrong way. It should be moved back. Done. https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IExtension.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IExtension.java: Line 27: enum ExtensionPoint { > Maybe call this "ExtensionKind"? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IMetadataEntity.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/api/IMetadataEntity.java: Line 24: public interface IMetadataEntity extends Serializable { > Why does this need to move to common? Isn't metadata the right package? I moved it here for two reasons. 1. ExternalFile is a metadata entity that lives in external data which comes before metadata. 2. not all of the metadata types implements the addToCache and removeFromCache. P.S we need to revisit metadata again one more time https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/AsterixExtensionProperties.java File asterixdb/asterix-common/src/main/java/org/apache/asterix/common/config/AsterixExtensionProperties.java: Line 29: public Extensions getExtensions() { > It would be nice if this JAXB-generated class would not spill outside the p Not sure I follow. Do you mean that getExtensions should produce a non JAXB generated class? https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-common/src/main/resources/schema/asterix-conf.xsd File asterixdb/asterix-common/src/main/resources/schema/asterix-conf.xsd: Line 114: <xs:element name="extensions"> > The indentation seems wrong. However, the bigger question is, if we need th Let's discuss? https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-lang-aql/src/main/java/org/apache/asterix/lang/aql/rewrites/AqlQueryRewriter.java File asterixdb/asterix-lang-aql/src/main/java/org/apache/asterix/lang/aql/rewrites/AqlQueryRewriter.java: Line 115: AQLInlineUdfsVisitor visitor = > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/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 1658: ExtensionMetadataIndex<T> index = (ExtensionMetadataIndex<T>) extensionIndexes.get(entity.getIndexId()); > Should these three function be next to the other three for readability? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/AbstractMetadataExtension.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/api/AbstractMetadataExtension.java: Line 28: public abstract class AbstractMetadataExtension implements IExtension { > Should we make this an interface? There's not much shared functionality her Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataIndex.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataIndex.java: Line 89: throw new IllegalArgumentException("Unequal number of key types and names given."); > Why are unchecked exceptions better here? I think that we talked about this Here is my reasoning for this. This constructor is ONLY being called during the system initialization to create some static members. If we change them to checked exceptions, then we can't call the constructor in the static member declaration. However, with unchecked exception, we can and we ensure that static members are final and initialized correctly. https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataIndexImmutableProperties.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataIndexImmutableProperties.java: Line 27: public static final int FIRST_AVAILABLE_EXTENSION_METADATA_DATASET_ID = 52; > Good question. What's special about 52? It is a power of 2 :) I think we should agree on a range that is reserved for extension datasets. then an extension shouldn't select its own dataset Id but rather request a dataset Id. However, I think we should do that in a subsequent change. Line 27: public static final int FIRST_AVAILABLE_EXTENSION_METADATA_DATASET_ID = 52; > We should at least go with the prime number, 53 :) We can use (2^74,207,281) − 1 aka largest known prime number ;) https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataPrimaryIndexes.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/bootstrap/MetadataPrimaryIndexes.java: Line 24: import org.apache.asterix.common.config.MetadataConstants; > Couldn't we have the MetadataConstants in the metadata project? I think that we should. let me see if that is feasible. https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/DatasetDataSource.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/DatasetDataSource.java: Line 38: throws AlgebricksException { > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/InternalDatasetDetails.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/InternalDatasetDetails.java: Line 158: AqlSerializerDeserializerProvider.INSTANCE.getSerializerDeserializer(BuiltinType.ABOOLEAN); > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/functions/ExternalFunctionCompilerUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/functions/ExternalFunctionCompilerUtil.java: Line 74: FunctionIdentifier fid = > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/SplitsAndConstraintsUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/SplitsAndConstraintsUtil.java: Line 49: AsterixClusterProperties.INSTANCE.getNodePartitionsCount(clusterPartition[j].getNodeId()); > Can we revert this file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-om/src/main/java/org/apache/asterix/om/util/AsterixAppContextInfo.java File asterixdb/asterix-om/src/main/java/org/apache/asterix/om/util/AsterixAppContextInfo.java: Line 68: private ICCExtensionManager extensionManager; > Yes! Done -- 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: 6 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
