asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdullah alamoudi (Code Review)" <>
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:

File asterix-app/src/main/java/org/apache/asterix/feed/

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.
File asterix-app/src/main/java/org/apache/asterix/hyracks/bootstrap/

Line 273:         //Clean any temporary files
> Revert the file?
Okay :)
File asterix-common/src/main/java/org/apache/asterix/common/config/

Line 130: 
> Revert the file?
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?
File asterix-external-data/src/main/java/org/apache/asterix/external/api/

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

Line 56:     public void setRecord(T t);
> The getter is only called "get", should we call this one "set"?
File asterix-external-data/src/main/java/org/apache/asterix/external/api/

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?
File asterix-external-data/src/main/java/org/apache/asterix/external/feed/message/

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.
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/

Line 29: import;
> This is an ugly dependency, can we move the "set" method that uses it as a 
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/

Line 58:     public Class<? extends T> getRecordClass() {
> It seems that we actually don't use this method.
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/

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 :-)
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/

Line 98:         record.setRecord(value);
> Oh, look, it was called "set"!
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/

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
File asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/factory/

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

Line 85:             throw new AsterixException("Unspesified Couchbase nodes");
> a/Unspesified/Unspecified/ ?
File asterix-external-data/src/main/java/org/apache/asterix/external/operators/

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.
File asterix-external-data/src/main/java/org/apache/asterix/external/parser/

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
File asterix-runtime/src/main/java/org/apache/asterix/runtime/operators/std/

Line 45:         };
> Revert the file?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id862ce9e9b1360864c6976f2aea2137092f51203
Gerrit-PatchSet: 9
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: abdullah alamoudi <>
Gerrit-Reviewer: Ian Maxon <>
Gerrit-Reviewer: Jenkins <>
Gerrit-Reviewer: Murtadha Hubail <>
Gerrit-Reviewer: Till Westmann <>
Gerrit-Reviewer: abdullah alamoudi <>
Gerrit-HasComments: Yes

View raw message