Till Westmann has posted comments on this change. Change subject: Add Asterix Extension Manager ......................................................................
Patch Set 6: (41 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 here. 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? 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? https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/RESTAPIServlet.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/http/servlet/RESTAPIServlet.java: Line 68: // Can the IQueryTranslatorFactory be a member of ILangCompilationProvider?! Should it be?! It seems that they complement each other so we probably shouldn't nest one into the other. 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 here. 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 this change ... 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 ... 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. Line 75: throw new IllegalArgumentException("Extension Conflict between " + extension.getId() It seems that we could share error codes and error messages in all switch cases. Line 111: IHyracksClientConnection hcc) throws HyracksDataException { It seems that we could just add a "handle" or "process" method to the IExtensionStatement interface. Then each statement could handle itself and this method would be much simpler. Also, we wouldn't need to care who created the statement. Longer term it might also be useful to move existing statements into that model. 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. https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/NCExtensionManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/nc/NCExtensionManager.java: Line 45: throw new IllegalArgumentException(); Can we throw a real exception here? 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 we avoid the ICCExtensionManager interface for now? 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 ... 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/ 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 we avoid the ICCExtensionManager interface for now? 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? 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? 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 we avoid the ICCExtensionManager interface for now? https://asterix-gerrit.ics.uci.edu/#/c/1017/6/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: Line 121: (ExternalDatasetDetails) dataset.getDatasetDetails(); Can we revert this file? 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? 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? 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? 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? 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"? 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? 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 properties accessors - the fact that we store some configuration in XML is just an implementation detail. 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 this in this schema ... 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? 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 135: private Map<ExtensionMetadataIndexId, ExtensionMetadataIndex<?>> extensionIndexes; Should those be extensionDatasets? 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 here. 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, but I forgot ... 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? Line 49: public String getDatasetName() { Can we restore the original order of these methods? 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? 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? 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? 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? https://asterix-gerrit.ics.uci.edu/#/c/1017/6/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/DatasetUtils.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/utils/DatasetUtils.java: Line 260: ISerializerDeserializer<AString> stringSerde = Can we revert this file? 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? 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; > MAJOR SonarQube violation: Yes! -- 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
