Till Westmann has posted comments on this change. Change subject: Add Asterix Extension Manager ......................................................................
Patch Set 13: (27 comments) One rounds of comments - didn't make it all the way through (had to fight with some build-problems ...) https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveManager.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveManager.java: Line 85: runtime.stop(); We're stopping the runtime for every message delivery? Line 101: LOGGER.warn("Failed to stop runtime: " + runtimeId, e); Is a warning enough here? Will the system continue to work as before? Or are we building up garbage? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeManager.java File asterixdb/asterix-active/src/main/java/org/apache/asterix/active/ActiveRuntimeManager.java: Line 64: public ActiveSourceOperatorNodePushable getFeedRuntime(ActiveRuntimeId runtimeId) { Method name seems wrong. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/base/ILangExtension.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/base/ILangExtension.java: Line 37: public enum Language { Not sure that we should limit the languages with an enum, but it's good enough for now. If somebody wants to add another language there's another opportunity to improve extension points ... https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/ExtensionFunctionIdentifier.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/ExtensionFunctionIdentifier.java: Line 27: public class ExtensionFunctionIdentifier extends FunctionIdentifier { It feels a little strange that we would need this. It would be nice if every function just had an identifier and we could get relevant properties from the function or via the identifier. Maybe we should file an issue to remove the ExtensionFunctionIdentifier at a later stage? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/algebra/extension/IExtensionStatement.java: Line 29: public interface IExtensionStatement extends Statement { Looks like a nice interface. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/FuzzyUtils.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/FuzzyUtils.java: Line 23: import org.apache.asterix.lang.common.util.FunctionUtils; I think that the number of modified files will go down a bit if we remove the 's' as well. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: Line 154: //TODO(amoudi/yingyi): refactor this to use a provider instead of passing the extensionManager I completely agree. Could you add JIRA issues for the big TODO's? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/IntroduceSecondaryIndexInsertDeleteRule.java: Line 794: FunctionUtils.getFunctionInfo(AsterixBuiltinFunctions.AND), filterExpressions)); Please remove the 's' :) https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/MetaFunctionToMetaVariableRule.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/MetaFunctionToMetaVariableRule.java: Line 101: } ; Don't need the space or the semicolon .. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/am/OptimizableOperatorSubTree.java: Line 112: } ; remove space and semicolon Line 233: } Would be nice to pull out ((AqlDataSource) dataSourceScan.getDataSource()).getDatasourceType() BTW, do we know that the cast to (AqlDataSource) will always succeed? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/AqlExpressionToPlanTranslator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/AqlExpressionToPlanTranslator.java: Line 37: import org.apache.asterix.lang.common.util.FunctionUtils; Probably not needed at all ... https://asterix-gerrit.ics.uci.edu/#/c/1017/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: Line 104: JobSpecification rewriteCompileQuery(AqlMetadataProvider metadataProvider, Query query, It seems that this is a sub-functionality of statement execution. Probably we should have a separate interface (IQueryCompiler?) for this - there could still be one class that implements both ... Maybe file an issue for that? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ResultPrinter.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ResultPrinter.java: Line 19: package org.apache.asterix.translator; Why would the ResultReader be part of the translator package? It's also unclear why it should be in the "algebra" module. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ResultReader.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ResultReader.java: Line 19: package org.apache.asterix.translator; Seems that the old package was better. Even though we currently have execution very close to translation it should be different - especially if the result is picked up asynchronously ... https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ResultUtils.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/ResultUtils.java: Line 54: .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue))); Funky. Why is this better? (Just curious) https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/ConstantExpressionUtils.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/util/ConstantExpressionUtils.java: Line 34: public class ConstantExpressionUtils { This will clash with the ConstantExpressionUtil in https://asterix-gerrit.ics.uci.edu/1074/. But we'll solve that once we get there ... https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/APIFramework.java: Line 98: private final CompilerExtensionManager ccExtensionManager; s/ccExtensionManager/cExtensionManager/? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/api/common/AsterixHyracksIntegrationUtil.java: Line 42: import org.apache.log4j.Logger; All good except for the use of log4j (ASTERIXDB-1564) ... https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 67: this.parserFactory = compilationProvider.getParserFactory(); WS Line 217: if (!(st.getAPI() == Statement.API.ALL || getAPI() == Statement.API.ALL || st.getAPI() == getAPI())) { That doesn't seem to make a lot of sense. Now the allowedStatements are usused .. What should this change achieve? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CompilerExtensionManager.java File asterixdb/asterix-app/src/main/java/org/apache/asterix/app/cc/CompilerExtensionManager.java: Line 115: SqlppCompilationProvider.class.getSimpleName()); Probably shouldn't be the SQL++ provider for any language. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 46: * WS Line 97: * WS https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 { I'm not sure what these operations are (maybe we could comment on them) but it seems that they are only need in asterix-algebra and asterix-app and so they could stay in asterix-metadata. Further, the relationship between the class name IMetadataEntity and the constants if not very clear (at least to me). Maybe we shouldn't touch this for this change? https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/base/Statement.java File asterixdb/asterix-lang-common/src/main/java/org/apache/asterix/lang/common/base/Statement.java: Line 44: public byte getAPI(); This is very confusing: 1) The comment is just copied. 2) It doesn't seem right to mix API information with the language here. We could consider statement categories (DDL, UPDATE, QUERY), but I don't know if that's sufficient for the use-case ... -- 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: 13 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
