abdullah alamoudi has posted comments on this change. Change subject: First stage of external data cleanup ......................................................................
Patch Set 8: (56 comments) https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-app/src/main/java/org/apache/asterix/file/ExternalIndexingOperations.java File asterix-app/src/main/java/org/apache/asterix/file/ExternalIndexingOperations.java: Line 208: conf.set("fs.default.name", map.get(ExternalDataConstants.KEY_HDFS_URL).trim()); > Change these to constants Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-common/src/main/java/org/apache/asterix/common/feeds/api/IDatasourceAdapter.java File asterix-common/src/main/java/org/apache/asterix/common/feeds/api/IDatasourceAdapter.java: Line 30: */ > rename to IDataSourceAdapter Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/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 66: private static int port; > Let's make those non-static while we're touching it. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/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 43: > There is no need to implement IFeedAdapterFactory. Done Line 59: public String getAlias() { > if this is going to be the only adapter, you may get rid of this field. We will still allow other user defined adapters and would like them to be usable using their aliases since class names tends to be long and not too user friendly. Line 88: private void configure() throws Exception { > try to change the name of this method. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalDataSourceFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalDataSourceFactory.java: Line 66: public boolean indexible(); > rename this to isIndexible() Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalIndexer.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IExternalIndexer.java: Line 25: public interface IExternalIndexer extends Serializable { > Please add java doc on these methods. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/api/IFeedClientFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IFeedClientFactory.java: Line 24: public interface IFeedClientFactory { > Delete this class. Done Line 28: > Please add TODOs to remove those completely. Class was deleted https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/api/IIndexibleExternalDataSource.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IIndexibleExternalDataSource.java: Line 33: public boolean indexing(); > The existing name of this method is confusing. It sounds like the datasourc Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/api/ILookupAdapter.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/ILookupAdapter.java: Line 27: * @author amoudi > ASF code usually does not contain author tags. One of the reasons is, that Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/api/IPullBasedFeedAdapter.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IPullBasedFeedAdapter.java: Line 25: > Either remove it and all of implementers or add a TODO to remove it. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/api/IRawRecord.java File asterix-external-data/src/main/java/org/apache/asterix/external/api/IRawRecord.java: Line 21: public interface IRawRecord<T> { > Add java docs here. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/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 26: > add java docs. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/AbstractDataFlowController.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/AbstractDataFlowController.java: Line 36: public AbstractDataFlowController() { > Remove this from here and all of its subclasses. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/IndexingDataFlowController.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataflow/IndexingDataFlowController.java: Line 30: public IndexingDataFlowController() throws HyracksDataException { > Remove this. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/dataset/adapter/GenericAdapter.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataset/adapter/GenericAdapter.java: Line 26: public class GenericAdapter implements IDatasourceAdapter, IFeedAdapter { > remove IFeedAdapter Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/dataset/adapter/LookupAdapter.java File asterix-external-data/src/main/java/org/apache/asterix/external/dataset/adapter/LookupAdapter.java: Line 109: this.writer = writer; > You may want to pass this in the constructor and implement IFrameWriter ins Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/FileOffsetIndexer.java File asterix-external-data/src/main/java/org/apache/asterix/external/indexing/FileOffsetIndexer.java: Line 46: private ISerializerDeserializer<AInt32> intSerde = AqlSerializerDeserializerProvider.INSTANCE > change to generic? :) Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/RecordColumnarIndexer.java File asterix-external-data/src/main/java/org/apache/asterix/external/indexing/RecordColumnarIndexer.java: Line 48: private ISerializerDeserializer<AInt32> intSerde = AqlSerializerDeserializerProvider.INSTANCE > generic? Done Line 57: HDFSRecordReader<?, Writable> hdfsReader = (HDFSRecordReader<?, Writable>) reader; > TODO? Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/RecordIdReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/indexing/RecordIdReader.java: Line 34: public final static byte nullByte = ATypeTag.NULL.serialize(); > private Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/indexing/RecordIdReaderFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/indexing/RecordIdReaderFactory.java: Line 34: return null; > throw invalid type exception Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/input/HDFSDataSourceFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/HDFSDataSourceFactory.java: Line 60: protected boolean configured = false; > remove configured if not used for state checks. Done Line 164: return (recordClass == null) ? DataSourceType.STREAM : DataSourceType.RECORDS; > change this to use same logic in configure. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/CharArrayRecord.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/CharArrayRecord.java: Line 91: if (value[size - 1] != '\n') { > replace \n by static value, or if java has another built in one. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/AbstractHDFSLookupRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/AbstractHDFSLookupRecordReader.java: Line 53: this.file = new ExternalFile("", "", -1, "", new Date(), 0L, ExternalFilePendingOp.PENDING_NO_OP); > create empty constrcutor. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/HDFSRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/HDFSRecordReader.java: Line 137: if (reader != null) { > remove this check. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/RSSRecordReader.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/RSSRecordReader.java: Line 132: System.err.println(feedUrl + " retrieved"); > Could you add TODO's to remove this writing to the error stream? Done Line 157: System.err.println("\tEVENT: Feed Polled. URL = " + event.getUrlString()); > Could you add TODO's to remove this writing to the error stream? Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/HDFSLookupReaderFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/HDFSLookupReaderFactory.java: Line 86: return null; > throw invalid format. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalLoopkupOperatorDiscriptor.java File asterix-external-data/src/main/java/org/apache/asterix/external/operators/ExternalLoopkupOperatorDiscriptor.java: Line 46: public class ExternalLoopkupOperatorDiscriptor extends AbstractTreeIndexOperatorDescriptor { > I think that there are 2 typos in this name and I suggest a correct spellin Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/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 156: return isStreamParser ? DataSourceType.STREAM : DataSourceType.RECORDS; > use same logic as in configure. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/parser/RSSParser.java File asterix-external-data/src/main/java/org/apache/asterix/external/parser/RSSParser.java: Line 46: public RSSParser() { > remove. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/parser/factory/AbstractRecordStreamParserFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/parser/factory/AbstractRecordStreamParserFactory.java: Line 32: * > remove. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/provider/DatasourceFactoryProvider.java File asterix-external-data/src/main/java/org/apache/asterix/external/provider/DatasourceFactoryProvider.java: Line 39: String stream = configuration.get(ExternalDataConstants.KEY_STREAM); > replace this to check for the type. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/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 52: return new HiveDataParserFactory(); > Add missing types and default to unsupported. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/runtime/GenericSocketFeedAdapter.java File asterix-external-data/src/main/java/org/apache/asterix/external/runtime/GenericSocketFeedAdapter.java: Line 34: > This class is expected to be removed. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/runtime/GenericSocketFeedAdapterFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/runtime/GenericSocketFeedAdapterFactory.java: Line 44: public class GenericSocketFeedAdapterFactory implements IFeedAdapterFactory { > This class is expected to be removed. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/runtime/SocketClientAdapter.java File asterix-external-data/src/main/java/org/apache/asterix/external/runtime/SocketClientAdapter.java: Line 30: > This class is expected to be removed. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/runtime/SocketClientAdapterFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/runtime/SocketClientAdapterFactory.java: Line 31: public class SocketClientAdapterFactory implements IFeedAdapterFactory { > This class is expected to be removed. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/runtime/TwitterFirehoseFeedAdapterFactory.java File asterix-external-data/src/main/java/org/apache/asterix/external/runtime/TwitterFirehoseFeedAdapterFactory.java: Line 72: this.configureFormat(outputType); > Remove this call and add TODO to set the correct parser. complete class was removed and replaced with TwitterFirehoseInputStreamProvider Line 75: private void configureFormat(ARecordType outputType2) { > Remove this method. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/util/DNSResolver.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/DNSResolver.java: Line 37: public class DNSResolver implements INodeResolver { > Update class name to reflect new functionality. Done Line 89: AsterixRuntimeUtil.getNodeControllerMap(ncMap); > Add a TODO to change this call and replace by calling AsterixClusterPropert Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/util/DataflowUtils.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/DataflowUtils.java: Line 52: } > if policyType == null, throw exception Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/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 51: public static void addCompatabilityParameters(String adapterClassname, ARecordType itemType, > Add the rest of the aliases TODO was added. Line 54: || adapterClassname.contains("HDFSAdapter")) { > Replace by final constant Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataConstants.java File asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataConstants.java: Line 130: public static final String ALIAS_TWITTR_FIREHOSE_ADAPTER = "twitter_firehose"; > TWITTER Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/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 90: return aString.contains(ExternalDataConstants.EXTERNAL_LIBRARY_SEPARATOR); > Add TODO to check for the format instead of contains only Done Line 118: // TODO Auto-generated method stub > complete the implementation. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-maven-plugins/lexer-generator-maven-plugin/src/main/resources/Lexer.java File asterix-maven-plugins/lexer-generator-maven-plugin/src/main/resources/Lexer.java: Line 92: public void setInputStream(java.io.Reader stream, char[] buffer) throws IOException{ > Remove. Done Line 109: public void removeInputStream(){ > Remove. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedIntakeOperatorNodePushable.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedIntakeOperatorNodePushable.java: Line 89: // if (adapterFactory.isRecordTrackingEnabled()) { > Add TODO here. Done https://asterix-gerrit.ics.uci.edu/#/c/566/8/asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedUtil.java File asterix-metadata/src/main/java/org/apache/asterix/metadata/feeds/FeedUtil.java: Line 481: System.out.println("adapter name: " + feed.getAdaptorName()); > This is temporary, right? Ouch. I added them when debugging. going to remove them :) DONE! -- To view, visit https://asterix-gerrit.ics.uci.edu/566 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: I04a8c4e494d8d1363992b6fe0bdbe6b2b3b7b767 Gerrit-PatchSet: 8 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Murtadha Hubail <hubail...@gmail.com> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com> Gerrit-HasComments: Yes
