Till Westmann has posted comments on this change. Change subject: Enabled Feed Tests and Added External Library tests ......................................................................
Patch Set 15: (61 comments) The story around FeedLogManager and AbstractFeedDataFlowController is a little unclear. It seems that those are breaking through too many interfaces and it also does not seem necessary. However, I also do see that changing this would keep us from getting fixes and test coverage earlier and so I think that it is ok to take some technical debt and file an issue for this to be addressed later. It would be nice if the unused member could be removed and if the licensing for the test files could be addressed. I didn't look at the implementation of the ClassAd parser. 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 under src/test/java) so that we can keep this method package local. Line 189: public static void installLibraryIfNeeded(String dataverse, final File libraryDir, Could be package local - see above 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 ... Line 210: LOGGER.info("Added library " + libraryName + "to Metadata"); space between '"' and 'to' Line 226: //throw new Exception("No library descriptor defined"); Remove commented code? Line 295: public static void registerLibrary(String dataverse, String libraryName, boolean isMetadataNode, File installLibDir) Could be package local - see above Line 390: public static File getLibraryInstallDir() { Could be package local - see above Line 398: public static File getLibraryUninstallDir() { Could be package local - see above 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 of the public interfaces in ExternalLibraryUtils. Line 60: // install if needed (i,e, add the functions, adapters, datasources, parsers to the metadata) <Not required for use> line break? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? 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? Otherwise this error message might be very confusing.. Maybe we can also file an issue, that this (and other) error message should get the user-visible name ... 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 comments (and licenses) in the test files. 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 here? Couldn't we just check if the factories are null? If we need the flags, could you add a comment? 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 doesn't seem to fit in here? Line 74: public void setFeedLogManager(FeedLogManager feedLogManager);; Same here, also there are 2 semi-colons. 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 it 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/ Line 76: throw new HyracksDataException(th); Why don't we close the forwarder here? Could you add a comment? 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. 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? 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? 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? 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. 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 ... 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. 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. 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 .. 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. 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. 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 controller or a log manager. I think that both are use to pass notifications to and maybe we could think about using a smaller interface here (at some point in the future ..). 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. 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. 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 ;) 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. 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. 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. Line 74: private AbstractFeedDataFlowController controller; Seems to be unused. 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: - no space before the exclamation mark - just one exclamation mark - a space after the colon 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 factories as we do for the other things we create in here? That'd be slightly more consistent and we could probably keep the FeedLogManager outside of this method. 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? 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 ... Line 135: public void logError(String error, Throwable th) throws IOException { Seems to be unused ... 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? Seems overly verbose here. 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? 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? -- 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
