abdullah alamoudi has posted comments on this change. Change subject: Support Change Feeds and Ingestion of Records with MetaData ......................................................................
Patch Set 11: (45 comments) https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/base/RuleCollections.java: Line 38: import org.apache.asterix.optimizer.rules.MetaFunctionToMetaVariableRule; > It seems the imports are not lexically ordered. Format the code? Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/MetaFunctionToMetaVariableRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/MetaFunctionToMetaVariableRule.java: Line 49: private boolean hasApplied = false; > Remove unnecessary finals. Done Line 74: final DataSourceScanOperator scanOp = (DataSourceScanOperator) op; > remove unnecessary finals. Done Line 250: this.fieldAccessExpressions = fieldAccessExpressions; > it's no longer field-access expressions, right? Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/UnnestToDataScanRule.java File asterix-algebra/src/main/java/org/apache/asterix/optimizer/rules/UnnestToDataScanRule.java: Line 349: } > Independent of where these methods will live, they could be a little shorte Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-algebra/src/main/java/org/apache/asterix/translator/CompiledStatements.java File asterix-algebra/src/main/java/org/apache/asterix/translator/CompiledStatements.java: Line 409: public String getFeedName() { > Seems to be unused. This one is used. Line 418: public FeedConnectionRequest getConnectionRequest() { > Seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java File asterix-algebra/src/main/java/org/apache/asterix/translator/LangExpressionToPlanTranslator.java: Line 91: import org.apache.asterix.optimizer.rules.UnnestToDataScanRule; > translator shouldn't depend on a rewriting rule. Done Line 196: "Unable to load dataset " + clffs.getDatasetName() + " since it is a read only dataset"); > I think that this is related to another discussion we had. The point of tha Yes. I remember Mike has suggested such an error message. We can consult with him. Line 228: UnnestToDataScanRule.prepareVarAndExpression(keyFieldName, payloadVar, pkVars, pkExprs, varRefsForLoading, > Move that method to a util class (in org.apache.asterix.translator.util) fr Done Line 253: UnnestToDataScanRule.prepareVarAndExpression(additionalFilteringField, payloadVar, additionalFilteringVars, > Move that method to a util class (in org.apache.asterix.translator.util) fr Done Line 374: + targetDatasource.getDataset().getDatasetName() + " since it a read only dataset"); > Change the wording, similar to load. Done Line 387: + targetDatasource.getDataset().getDatasetName() + " since it a read only dataset"); > Change the wording, similar to load. Done Line 408: + targetDatasource.getDataset().getDatasetName() + " since it a read only dataset"); > Change the wording, similar to load. Done Line 449: project.getVariables().add(metaAndKeysAssign.getVariables().get(0)); > "metaAndKeysAssign.getVariables().get(0)" --> metaVar Done Line 455: new ArrayList<Mutable<ILogicalExpression>>()); > Why this empty assign is needed? Because, then it means that the assign was not instantiated and so we will get NPE when we add new variables/expressions. Did some refactoring to move it before creating meta var and expression. Line 464: UnnestToDataScanRule.replaceMetaWithMetaKey(assignExpr); > In principle, the translator shouldn't have dependency to rewriting rules.. Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-app/src/test/java/org/apache/asterix/test/optimizer/OptimizerTest.java File asterix-app/src/test/java/org/apache/asterix/test/optimizer/OptimizerTest.java: Line 142: final String queryFileShort = queryFile.getPath().substring(PATH_QUERIES.length()) > remove unnecessary final. Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_02/feeds_02.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_02/feeds_02.1.ddl.aql: Line 20: * Description : Create a feed dataset that uses the feed simulator adapter. > WS Done Line 21: Begin ingestion and verify contents of the dataset post completion. > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-app/src/test/resources/runtimets/testsuite.xml File asterix-app/src/test/resources/runtimets/testsuite.xml: Line 128: <!-- > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-common/src/main/java/org/apache/asterix/common/config/AsterixPropertiesAccessor.java File asterix-common/src/main/java/org/apache/asterix/common/config/AsterixPropertiesAccessor.java: Line 49: private static Logger LOGGER = Logger.getLogger(AsterixPropertiesAccessor.class.getName()); > Revert file? I think these are not that bad? https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/api/IDataFlowController.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IDataFlowController.java: Line 26: public void start(IFrameWriter writer) throws HyracksDataException; > I guess stop(), pause(), and resume() probably would better present in the Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/api/IDataParserFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IDataParserFactory.java: Line 60: public void setMetaType(ARecordType metaType); > Do we really need the two set methods? I agree. However, these are extendable classes that would be created using classForName().newInstance(); https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/api/IStreamDataParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IStreamDataParser.java: Line 56: public default void appendKeys(final ArrayTupleBuilder tb) throws IOException { > Seems unused, can we remove it? Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/CounterTimerTupleForwarder.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/CounterTimerTupleForwarder.java: Line 55: public CounterTimerTupleForwarder(int batchSize, long batchInterval) { > Make this one private, if we have a factory method? Done Line 149: public static CounterTimerTupleForwarder create(Map<String, String> configuration) { > Move the factory method closer to the constructor? (Or put this code into t Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/RateControlledTupleForwarder.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/RateControlledTupleForwarder.java: Line 43: public RateControlledTupleForwarder(long interTupleInterval) { > Same comments as for CounterTimerTupleForwarder. Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameDistributor.java File asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FrameDistributor.java: Line 38: private final static Logger LOGGER = Logger.getLogger(FrameDistributor.class.getName()); > revert file? Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/SocketClientInputStreamProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/SocketClientInputStreamProvider.java: Line 96: public void setFeedLogManager(FeedLogManager feedLogManager) { > This is apparently never used. Will we need it in the future? We need to figure that out. I need to give data source classes a way to write feed logs. I don't think this is the right way but we need to figure the right way out. In a subsequent change? I have already created a JIRA issue for this. https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/provider/LookupReaderFactoryProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/provider/LookupReaderFactoryProvider.java: Line 34: final String inputFormat = HDFSUtils.getInputFormatClassName(configuration); > unnecessary finals. Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/provider/ParserFactoryProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/provider/ParserFactoryProvider.java: Line 41: final String parserFactoryName = configuration.get(ExternalDataConstants.KEY_DATA_PARSER); > unnecessary final Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataUtils.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataUtils.java: Line 127: throw new AsterixException(e); > Some additional message here would be nice ... Done Line 233: if (libraryAndFactory[0].contains(".")) { > if (! ...) throw, after that declare and initialize variables. Done Line 244: throw new AsterixException(e); > Some additional message here would be nice ... Done Line 259: throw new AsterixException(e); > Some additional message here would be nice ... Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-external-data/src/main/java/org/apache/asterix/external/util/TweetGenerator.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/TweetGenerator.java: Line 35: private static final Logger LOGGER = Logger.getLogger(TweetGenerator.class.getName()); > revert file? Is it this bad? I would like to get rid of the exception in the constructor declaration and might as well make members which are not expected to change final. https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-installer/src/test/resources/integrationts/library/queries/library-feeds/feed_ingest/feed_ingest.1.ddl.aql File asterix-installer/src/test/resources/integrationts/library/queries/library-feeds/feed_ingest/feed_ingest.1.ddl.aql: Line 55: create dataset TweetsFeedIngest(TweetOutputType) > WS Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-installer/src/test/resources/integrationts/lifecycle/results/asterix-lifecycle/backupRestore/backupRestore.1.adm File asterix-installer/src/test/resources/integrationts/lifecycle/results/asterix-lifecycle/backupRestore/backupRestore.1.adm: Line 1: { "DataverseName": "backupDataverse", "DataFormat": "org.apache.asterix.runtime.formats.NonTaggedDataFormat", "Timestamp": "Wed Apr 24 16:13:46 PDT 2013", "PendingOp": 0i32 } > Why did this format change? The format didn't change but before, we didn't check values of fields that come after the first time field. This should've been fixed the time we made int64 the default. https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/AqlMetadataProvider.java: Line 386: FeedCollectOperatorDescriptor feedCollector = null; > Could also be declared where it's written. Done Line 391: RecordDescriptor feedDesc; > Move declaration to initialization? Could even make it final :) Done Line 401: ISerializerDeserializer serde = AqlSerializerDeserializerProvider.INSTANCE > inline "serde"? Done Line 407: FeedPolicyEntity feedPolicy = (FeedPolicyEntity) (feedDataSource).getProperties() > Why do we need parens around "feedDataSource"? Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/FeedDataSource.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/declared/FeedDataSource.java: Line 134: public List<Integer> getKeySourceIndicator() { > This method seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/621/11/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java File asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/AsterixLSMPrimaryUpsertOperatorNodePushable.java: Line 228: } catch (Exception e) { > remove this try-catch? Done -- To view, visit https://asterix-gerrit.ics.uci.edu/621 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: If136a03d424970132dfb09f0dda56e160d4c0078 Gerrit-PatchSet: 11 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ildar Absalyamov <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Steven Jacobs <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Yingyi Bu <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
