asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add Asterix Extension Manager
Date Tue, 16 Aug 2016 13:49:12 GMT
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 <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Michael Blow <mblow@apache.org>
Gerrit-Reviewer: Steven Jacobs <sjaco002@ucr.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Yingyi Bu <buyingyi@gmail.com>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message