abdullah alamoudi has posted comments on this change. Change subject: Enabled Feed Tests and Added External Library tests ......................................................................
Patch Set 15: (61 comments) https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java File asterix-app/src/main/java/org/apache/asterix/app/external/ExternalLibraryUtils.java: Line 137: public static boolean uninstallLibrary(String dataverse, String libraryName) > We should put the TestLibrarian into the same java package (of course still Done Line 189: public static void installLibraryIfNeeded(String dataverse, final File libraryDir, > Could be package local - see above Done Line 202: // Another place which shows that our metadata transactions are broken (we didn't call commit before!!!) > Do we need to keep this comment - it's fixed now ... I'd rather leave it since I believe metadata transaction are still broken since this used to work. Just another reminder to take a look at metadata transactions. Line 210: LOGGER.info("Added library " + libraryName + "to Metadata"); > space between '"' and 'to' Done Line 226: //throw new Exception("No library descriptor defined"); > Remove commented code? Done Line 295: public static void registerLibrary(String dataverse, String libraryName, boolean isMetadataNode, File installLibDir) > Could be package local - see above Done Line 390: public static File getLibraryInstallDir() { > Could be package local - see above Done Line 398: public static File getLibraryUninstallDir() { > Could be package local - see above Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/java/org/apache/asterix/test/common/TestLibrarian.java File asterix-app/src/test/java/org/apache/asterix/test/common/TestLibrarian.java: Line 34: public class TestLibrarian implements ITestLibrarian { > If we move this to org.apache.asterix.app.external, we can avoid a number o Done Line 60: // install if needed (i,e, add the functions, adapters, datasources, parsers to the metadata) <Not required for use> > line break? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser/classad-parser.2.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser/classad-parser.2.lib.aql: Line 1: install externallibtest testlib src/test/resources/externallib/testlib-zip-binary-assembly.zip > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser/classad-parser.3.ddl.aql File asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser/classad-parser.3.ddl.aql: Line 24: ("reader"="adm"), > Why is the reader "adm"? Is that what the parser translates the input to? this means that the input is semi-structured. we can use reader=semi-structured as well. I have changed it https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser/classad-parser.5.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser/classad-parser.5.lib.aql: Line 1: uninstall externallibtest testlib > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser2/classad-parser2.2.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser2/classad-parser2.2.lib.aql: Line 1: install externallibtest testlib src/test/resources/externallib/testlib-zip-binary-assembly.zip > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser2/classad-parser2.5.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/classad-parser2/classad-parser2.5.lib.aql: Line 1: uninstall externallibtest testlib > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/getCapital/getCapital.2.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/getCapital/getCapital.2.lib.aql: Line 1: install externallibtest testlib src/test/resources/externallib/testlib-zip-binary-assembly.zip > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/getCapital/getCapital.4.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/getCapital/getCapital.4.lib.aql: Line 1: uninstall externallibtest testlib > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.2.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.2.lib.aql: Line 1: install externallibtest testlib src/test/resources/externallib/testlib-zip-binary-assembly.zip > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.6.lib.aql File asterix-app/src/test/resources/runtimets/queries/external-library/typed_adapter/typed_adapter.6.lib.aql: Line 1: uninstall externallibtest testlib > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.2.lib.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.2.lib.aql: Line 1: install externallibtest testlib src/test/resources/externallib/testlib-zip-binary-assembly.zip > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.6.lib.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feed-with-external-parser/feed-with-external-parser.6.lib.aql: Line 1: uninstall externallibtest testlib > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_06/feeds_06.3.sleep.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_06/feeds_06.3.sleep.aql: Line 1: 3000 > Can we have a license here? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_07/feeds_07.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_07/feeds_07.1.ddl.aql: Line 55: > Why is the index gone? because the field doesn't exists and so it will simply fail. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_08/feeds_08.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_08/feeds_08.1.ddl.aql: Line 46: latitude: double, > Could you fix all the whitespace in this file? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/queries/feeds/feeds_09/feeds_09.1.ddl.aql File asterix-app/src/test/resources/runtimets/queries/feeds/feeds_09/feeds_09.1.ddl.aql: Line 46: latitude: double, > Same here, could you fix the whitespace? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-app/src/test/resources/runtimets/testsuite.xml File asterix-app/src/test/resources/runtimets/testsuite.xml: Line 32: <!-- Fails constantly and not clear what is intended > Could you file issues for the failing tests? There is already an issue that has a comment about them. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-common/src/main/java/org/apache/asterix/common/context/DatasetLifecycleManager.java File asterix-common/src/main/java/org/apache/asterix/common/context/DatasetLifecycleManager.java: Line 149: throw new HyracksDataException("Cannot remove index " + iInfo.datasetId > I guess that the datasetId here actually is an index id. Is that right? Oth Actually, this one should be reverted. I added it while debugging something :) https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java File asterix-common/src/test/java/org/apache/asterix/test/aql/TestExecutor.java: Line 647: case "lib": // expected format <dataverse-name> <library-name> <library-directory> > I think that we could filter out lines starting with a '#' here to support Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/adapter/factory/GenericAdapterFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/adapter/factory/GenericAdapterFactory.java: Line 55: private boolean parserFactoryCreated = true; > There's probably a good reason, but I don't see why we need extras flags in Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IRecordReader.java: Line 25: import org.apache.asterix.external.util.FeedLogManager; > I'm not sure what a better solution is, but do we agree that this import do Added a todo to find a better way to do this. Line 74: public void setFeedLogManager(FeedLogManager feedLogManager);; > Same here, also there are 2 semi-colons. Added a todo to find better place for flushing frames and perform logging Done. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/AbstractFeedDataFlowController.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/AbstractFeedDataFlowController.java: Line 42: this.feedLogManager = feedLogManager; > This is only used/needed in the FeedRecordDataFlowController. Can we keep i Both FeedRecordDataFlowController and FeedStreamRecordDataFlowController should use it. the thing is that all currently "tested" feeds use FeedRecordDataFlowController and so it was only used there. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedRecordDataFlowController.java: Line 75: LOGGER.warn("Failure during while operating a feed sourcec", th); > s/sourcec/source/ Done Line 76: throw new HyracksDataException(th); > Why don't we close the forwarder here? Could you add a comment? The reason is that if the throwable was caused by a bad record, we will come back to this loop and we want the forwarder to be closed later. Now that you mentioned it, If the adapter doesn't handle the exception, it will never be closed. I have fixed this. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedStreamDataFlowController.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedStreamDataFlowController.java: Line 34: super(feedLogManager); > Doesn't seem to be needed - but maybe I'm missing something. It should be needed. Unfortunately, we don't have yet feed tests that use stream parsers!. I will keep it for now but probably this whole class will go away. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedTupleForwarder.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/FeedTupleForwarder.java: Line 43: public static final int MAX_RECORD_SIZE = 16000; // temporary until the big object in storage is solved > But isn't that number frame-size-dependent? yes but this was a quick way to handle the big record case. I have fixed it now. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedFrameSpiller.java File asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedFrameSpiller.java: Line 164: } > Revert the file? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java File asterix-external-data/src/main/java/org/apache/asterix/external/feed/dataflow/FeedRuntimeInputHandler.java: Line 282: // TODO: fix handling of eod case with monitored buffers. > WS, add an issue? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/couchbase/CouchbaseReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/couchbase/CouchbaseReader.java: Line 84: private FeedLogManager feedLogManager; > This seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/hdfs/HDFSRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/hdfs/HDFSRecordReader.java: Line 62: private FeedLogManager feedLogManager; > This seems to be unused ... Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/rss/RSSRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/rss/RSSRecordReader.java: Line 57: private FeedLogManager feedLogManager; > This seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/AbstractStreamRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/AbstractStreamRecordReader.java: Line 44: protected FeedLogManager feedLogManager; > This seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/EmptyLineSeparatedRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/EmptyLineSeparatedRecordReader.java: Line 30: private int recordNumber = 0; > It seems that this is updated, but never read .. That is right. It was created for progress tracking which is still not yet completed. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPullRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPullRecordReader.java: Line 49: private FeedLogManager feedLogManager; > This seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPushRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/twitter/TwitterPushRecordReader.java: Line 43: private FeedLogManager feedLogManager; > This seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/AInputStream.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/AInputStream.java: Line 35: public abstract void setController(AbstractFeedDataFlowController controller); > It is a little confusing why an input stream needs a full data flow control I agree and I am not happy with it either. but it works for now :) Added a todo https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/AInputStreamReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/AInputStreamReader.java: Line 29: private FeedLogManager feedLogManager; > Seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/LocalFileSystemInputStream.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/LocalFileSystemInputStream.java: Line 36: private FeedLogManager logManager; > Seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/factory/TwitterFirehoseStreamProviderFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/factory/TwitterFirehoseStreamProviderFactory.java: Line 95: TwitterFirehoseInputStreamProvider streamProvider = new TwitterFirehoseInputStreamProvider(configuration, ctx, > If we avoid the variable, we could get it into one line ;) Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/LocalFSInputStreamProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/LocalFSInputStreamProvider.java: Line 37: private FeedLogManager feedLogManager; > Seems to be unused. It is used. we pass it to the input stream which in turns passes it to the file system watcher :) https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/SocketInputStreamProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/SocketInputStreamProvider.java: Line 32: private Map<String, String> configuration; > Both seem to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/TwitterFirehoseInputStreamProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/stream/provider/TwitterFirehoseInputStreamProvider.java: Line 53: private FeedLogManager feedLogManager; > Both seem to be unused. Done Line 74: private AbstractFeedDataFlowController controller; > Seems to be unused. Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/parser/ADMDataParser.java: Line 573: "This record is closed, you can not add extra fields !! new field name:" + fldName); > Proposals: Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/provider/DataflowControllerProvider.java: Line 69: Map<String, String> configuration, boolean indexingOp, boolean isFeed, FileSplit[] feedLogFileSplits) > Instead of having the last 3 parameters, couldn't we just pass in some fact I think that would work but it will be a larger change. hence, I will add a TODO instead. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataCompatibilityUtils.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataCompatibilityUtils.java: Line 76: // If reader is specified, we will use the selected reader. If format is specified, we will assign a suitable reader for the format. > line break? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedLogManager.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedLogManager.java: Line 67: public void endPartition(String partition) throws IOException { > Seems to be unused ... Right but I would really like to keep it until I am done with the at least once policy. I have a plan to use it later. Line 135: public void logError(String error, Throwable th) throws IOException { > Seems to be unused ... Right but I would really like to keep it until I am done with the at least once policy. I have a plan to use it later. https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedUtils.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/FeedUtils.java: Line 96: } > Couldn't this be part of constructing or opening the FeedLogManager? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-external-data/src/test/java/org/apache/asterix/external/classad/AMutableCharArrayString.java File asterix-external-data/src/test/java/org/apache/asterix/external/classad/AMutableCharArrayString.java: Line 87: this.length--; > Seems that we only need this once? Done https://asterix-gerrit.ics.uci.edu/#/c/625/15/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedMetadataUtil.java: Line 167: oldNewOID.put(opDesc.getOperatorId(), metaOp.getOperatorId()); > No more inverted indexes? I don't think that wrapping "non-primary" insert delete operators with feed store operator is a good idea and so, I have changed this to only wrap the first operator (the primary index insert delete operator) -- To view, visit https://asterix-gerrit.ics.uci.edu/625 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idd1fccd136fa2645b2707bbf7c04e60991ae8d4a Gerrit-PatchSet: 15 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
