asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Murtadha Hubail (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: First stage of external data cleanup
Date Sat, 02 Jan 2016 16:11:48 GMT
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 <bamousaa@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Murtadha Hubail <hubailmor@gmail.com>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message