Murtadha Hubail has posted comments on this change. Change subject: First stage of external data cleanup ......................................................................
Patch Set 8: (32 comments) First round of comments. comments on classes I couldn't open in gerrit: AsterixSocketInputStreamProvider.java remove throws from constructor ILookupRecordReader.java //add java docs IInputStreamProviderFactory.java public String getName(); IIndexingAdapterFactory.java Add java docs. 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 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 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. Line 59: public String getAlias() { if this is going to be the only adapter, you may get rid of this field. Line 88: private void configure() throws Exception { try to change the name of this method. 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() 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. 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. 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 datasource is *currently* indexing. I think it should be changed. 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. 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. 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. 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. 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. 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 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 instead. 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? :) 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? Line 57: HDFSRecordReader<?, Writable> hdfsReader = (HDFSRecordReader<?, Writable>) reader; TODO? 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 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 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. Line 164: return (recordClass == null) ? DataSourceType.STREAM : DataSourceType.RECORDS; change this to use same logic in configure. 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. 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. 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. Line 172: System.err.println("Something is not right"); remove this. 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. 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. 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. 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. 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. -- 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
