asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: Add flush() to IFrameWriter
Date Fri, 29 Jan 2016 09:08:26 GMT
abdullah alamoudi has posted comments on this change.

Change subject: Add flush() to IFrameWriter
......................................................................


Patch Set 9:

(19 comments)

https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-app/src/main/java/org/apache/asterix/feed/FeedMessageReceiver.java
File asterix-app/src/main/java/org/apache/asterix/feed/FeedMessageReceiver.java:

Line 95:     }
> What does this do?
Here is why this was added :-)
flush() calls don't cross the network boundaries and so, we need a way to trigger flush()
at the beginning of each task.

To achieve this, the message receiver for collect runtimes gets a notification whenever there
are no more messages indicating that it is time to flush() the content to storage.

In this specific receiver, we don't do anything.


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplicationEntryPoint.java
File asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/NCApplicationEntryPoint.java:

Line 273:         //Clean any temporary files
> Revert the file?
Okay :)


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-common/src/main/java/org/apache/asterix/common/config/AsterixPropertiesAccessor.java
File asterix-common/src/main/java/org/apache/asterix/common/config/AsterixPropertiesAccessor.java:

Line 130: 
> Revert the file?
Done


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-external-data/pom.xml
File asterix-external-data/pom.xml:

Line 279:         </dependency>
> Are these MIT licensed? And do the POMs have the correct metadata about the
I am not sure how to check whether they have the correct metadata about licenses.

How can we do that?


https://asterix-gerrit.ics.uci.edu/#/c/585/9/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 40:     public Class<?> getRecordClass();
> It seems that this is never used inside the code base. Do you know why we n
Yes. we actually don't need this one and Murtadha and I have had discussions about removing
it.
Done


Line 56:     public void setRecord(T t);
> The getter is only called "get", should we call this one "set"?
Done.


https://asterix-gerrit.ics.uci.edu/#/c/585/9/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 61:     public Class<? extends T> getRecordClass() throws IOException;
> This seems to be unused in our code. Do you know why we need it?
Removed :)


Line 73:     public default void setController(AbstractFeedDataFlowController controller)
throws UnsupportedOperationException {
> Why is this not IDataFlowController? This is an interface package, right?
Because we only do this with feeds in order to allow datasources to call flush() when needed.
Hence, there is no harm in being more specific?


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-external-data/src/main/java/org/apache/asterix/external/feed/message/MessageReceiver.java
File asterix-external-data/src/main/java/org/apache/asterix/external/feed/message/MessageReceiver.java:

Line 83:                 try {
> Why is polling and then calling emptyInbox() necessary here?
because we need a way to notify the receiver when there are no more messages in order to trigger
a flush() and not have records sitting in frames in memory.


https://asterix-gerrit.ics.uci.edu/#/c/585/9/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 29: import com.couchbase.client.deps.io.netty.buffer.ByteBuf;
> This is an ugly dependency, can we move the "set" method that uses it as a 
Done


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/GenericRecord.java
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/GenericRecord.java:

Line 58:     public Class<? extends T> getRecordClass() {
> It seems that we actually don't use this method.
Done


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/CouchbaseReader.java
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/CouchbaseReader.java:

Line 141:                 if (event.partialUpdate()) {
> I assume that this is just some temporary emptyness, right?
that is right as I still don't understand this well enough :-)


https://asterix-gerrit.ics.uci.edu/#/c/585/9/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 98:         record.setRecord(value);
> Oh, look, it was called "set"!
Done


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/AbstractStreamRecordReaderFactory.java
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/AbstractStreamRecordReaderFactory.java:

Line 19: package org.apache.asterix.external.input.record.reader.factory;
> Hmm, maybe it would make more sense to organize the reader package by reade
Done


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/CouchbaseReaderFactory.java
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/CouchbaseReaderFactory.java:

Line 82:             throw new AsterixException("Unspesified bucket");
> a/Unspesified/Unspecified/ ?
Done


Line 85:             throw new AsterixException("Unspesified Couchbase nodes");
> a/Unspesified/Unspecified/ ?
Done


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedMetaComputeNodePushable.java
File asterix-external-data/src/main/java/org/apache/asterix/external/operators/FeedMetaComputeNodePushable.java:

Line 190:                             } catch (InterruptedException e) {
> Why would we ignore this? Somebody called Thread.interrupt().
I agree that it shouldn't be ignored. I think I copied this code from somewhere where the
exception was ignored.


https://asterix-gerrit.ics.uci.edu/#/c/585/9/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 570:         if (recType != null) {
> Why is datasetRec not necessary anymore?
I don't know why it was introduced in the first place. just a quick look at the code before
shows that having datasetRec didn't make any difference in the behavior


https://asterix-gerrit.ics.uci.edu/#/c/585/9/asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/std/NoTupleSourceRuntimeFactory.java
File asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/std/NoTupleSourceRuntimeFactory.java:

Line 45:         };
> Revert the file?
Done


-- 
To view, visit https://asterix-gerrit.ics.uci.edu/585
To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id862ce9e9b1360864c6976f2aea2137092f51203
Gerrit-PatchSet: 9
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <bamousaa@gmail.com>
Gerrit-Reviewer: Ian Maxon <imaxon@apache.org>
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