abdullah alamoudi has posted comments on this change. Change subject: Add Asterix Extension Manager ......................................................................
Patch Set 13: (49 comments) 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? Done @Steven, Agree. will add it when there is a use case for it. Line 101: LOGGER.warn("Failed to stop runtime: " + runtimeId, e); > Is a warning enough here? Will the system continue to work as before? Or ar Done 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. Done 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 eno Done 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 ever Till, this was needed for the extension manager to know which extension should take care of compiling the function. 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 t Done 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? Already did :) 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' :) Done 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 .. Done 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 Done 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 ... Done 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 Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterixdb/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: Line 534: InsertDeleteUpsertOperator feedModificationOp; > I'm a little confused about how this change relates to extensions This is not extension related. It is generic change that is intended to replace the handle subscribe feed which will go away eventually. 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 unc You know the dependency story now :) Done. 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 execut You know the dependency story now :) Done. 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) because we're getting rid of the static block? Initialization is now tied with declaration. This means that no one will attempt to access the HTML_ENTITIES before it is initialized. Also, prevents anyone from modifying the list at a later point in time. 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.i Done 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/? Done 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) ... Done 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 Done 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 usu In order for the servlets to allow extension messages, we need the statement itself to identify itself as belonging to an API type. I think that we should get rid of the allowed statements altogether. 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. Done 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 Done Line 97: * > WS Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 384: // No op > What is this for? Historical reasons! https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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()))); > revert file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/HDFSCluster.java File asterixdb/asterix-app/src/test/java/org/apache/asterix/test/runtime/HDFSCluster.java: Line 38: * @author ramangrover29 > ws Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 55: * @param property > ws Done 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 Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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"> > Indent? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-common/src/main/resources/webui/errortemplate.html File asterixdb/asterix-common/src/main/resources/webui/errortemplate.html: Line 19: <div class="accordion" id="errorblock"> > Would be really nice if this were not part of asterix-common ... Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-common/src/main/resources/webui/static/js/smoothie.js File asterixdb/asterix-common/src/main/resources/webui/static/js/smoothie.js: Line 37: * resetBoundsInterval: 3000 > But that's not our code ... Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/SubscribableRuntime.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/feed/runtime/SubscribableRuntime.java: Line 46: @Override > ws Done 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: Till, the only reason I have added this is to enable extension statements to go through existing servlets. An alternative would be to treat all extension statements the same Line 44: public byte getAPI(); > This is very confusing: But with this, I can add new statements and let them go through the existing servlets. Otherwise, I will have to create new servlets. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj File asterixdb/asterix-lang-sqlpp/src/main/javacc/SQLPP.jj: Line 2916: | <COLLECTION : "collection"> > sqlpp is switching to dataset from table? Yes. There were some discussions about this. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 11: > ws Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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."); > Is this something that can ever happen in a working system? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlDataSource.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlDataSource.java: Line 232: throws AlgebricksException; > I'm not sure that this is the right abstraction. Clearly data seource and s absolutely. This was just to enable extension data sources to have their own scan runtime implementation. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/IMutationDataSource.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/IMutationDataSource.java: Line 26: public interface IMutationDataSource { > What is a MutationDataSource? A data source that produces values and operations<Deletes/Inserts/Updates>!!! https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/entities/Dataset.java: Line 37: public static final byte UPSERT = 0x03; > What are these constants? Can we explain them? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java File asterixdb/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java: Line 296: public static void increaseCardinality(JobSpecification spec, FeedRuntimeType compute, int requiredCardinality, > Method seems to be unused - but maybe we'll need it later? if we need it later, we will dig it up. Line 305: int requiredCardinality, List<String> currentLocations) throws AsterixException { > Method seems to be unused - but maybe we'll need it later? if we need it later, we will dig it up. Line 312: throws AsterixException { > Method seems to be unused - but maybe we'll need it later? if we need it later, we will dig it up. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 the file? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/job/listener/MultiTransactionJobEventListenerFactory.java File asterixdb/asterix-runtime/src/main/java/org/apache/asterix/runtime/job/listener/MultiTransactionJobEventListenerFactory.java: Line 34: public class MultiTransactionJobEventListenerFactory implements IJobletEventListenerFactory { > Some comment what this class does? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/ActivityId.java File hyracks-fullstack/hyracks/hyracks-api/src/main/java/org/apache/hyracks/api/dataflow/ActivityId.java: Line 52: public void setOperatorDescriptorId(OperatorDescriptorId odId) { > Do we really need to reset this? Or can we ensure that this is only set onc All of this is to enable combining multiple jobs into one. We totally do need this unless we're willing to do any of the following: 1. make ActivityId a non-final member in AbstractSingleActivityOperatorDescriptor and mutate it. 2. create a clone function in the AbstractSingleActivityOperatorDescriptor which will take an activity id and create a clone with the new Activity Id. This will then either be implemented by all subclasses of AbstractSingleActivityOperatorDescriptor or we can create a wrapper for it. let me know if I should try one of those. https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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 147: public ConnectorDescriptorId clone(IConnectorDescriptorRegistry registry); > Why would the clone of an IConnectorDescriptor be a ConnectorDescriptorId? Done https://asterix-gerrit.ics.uci.edu/#/c/1017/13/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); > Could we avoid resetting ids? When do we need that? We need that when we combine multiple jobs. We can instead clone operator descriptors. -- 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
