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 <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
