Till Westmann has posted comments on this change. Change subject: Add record reader and parser for CAP messages ......................................................................
Patch Set 12: (10 comments) Just a few comments - didn't think about your object allocation question ... https://asterix-gerrit.ics.uci.edu/#/c/1269/12/asterixdb/asterix-app/src/test/resources/runtimets/queries/feeds/caps-adaptor-collection/caps-adaptor-collection.4.update.aql File asterixdb/asterix-app/src/test/resources/runtimets/queries/feeds/caps-adaptor-collection/caps-adaptor-collection.4.update.aql: Line 18: */ Should we just leave this file out and renumber the following one(s)? I know that we have such empty files in other tests, but maybe we don't need to add more of those .. https://asterix-gerrit.ics.uci.edu/#/c/1269/12/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/CAPMessageRecordReader.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/record/reader/stream/CAPMessageRecordReader.java: Line 63: // TODO: simplify the state numbers (xikui) It would be better to use named constants instead of numbers for the state. Also it seems that "curState" or just "state" would be a better name than "curStatus". https://asterix-gerrit.ics.uci.edu/#/c/1269/12/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/CAPMessageParser.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/CAPMessageParser.java: Line 75: private SAXParserFactory capMessageSAXParserFactory; Could be a local variable. Line 130: rbList.get(0).write(out, true); Do you expect that parsing changes the value of rbList.get(0)? If not, we could have a local variable for the record builder instead of accessing the list every time. Line 244: out.write(BuiltinType.AINT32.getTypeTag().serialize()); This looks strange. Why do we need to serialize the type tag after the value here? Line 329: private void handleNestedOrderedList(String fullPathName, int fieldNameIdx) throws HyracksDataException { Could you add a comment to this method that describes the case in which it is needed? https://asterix-gerrit.ics.uci.edu/#/c/1269/12/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/factory/CAPMessageParserFactory.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/parser/factory/CAPMessageParserFactory.java: Line 31: import org.apache.hyracks.api.exceptions.HyracksDataException; Some imports seem to be unused. Could you remove them? https://asterix-gerrit.ics.uci.edu/#/c/1269/12/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataConstants.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/ExternalDataConstants.java: Line 206: public static final char QUESTION_MARK = '?'; It seems that these 4 are not used. Can we remove them again? https://asterix-gerrit.ics.uci.edu/#/c/1269/12/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/FileSystemWatcher.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/FileSystemWatcher.java: Line 240: } catch (InterruptedException x) { I'm not completely aware of the lifecycle here, but it seems that not setting the interrupted state isn't right. @Abdullah: Can you give some insight on how this should work? https://asterix-gerrit.ics.uci.edu/#/c/1269/12/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/LocalFileSystemUtils.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/util/LocalFileSystemUtils.java: Line 71: private static boolean fileNotExistsInList(LinkedList<File> files, Path path) { Should we use List<> instead of LinkedList<>? It seems that the chosen implementation should be immaterial to the interface. -- To view, visit https://asterix-gerrit.ics.uci.edu/1269 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia36101a0761973a9edb96b42d3dcc117661301da Gerrit-PatchSet: 12 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <[email protected]> Gerrit-Reviewer: Ian Maxon <[email protected]> Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]> Gerrit-Reviewer: Xikui Wang <[email protected]> Gerrit-Reviewer: abdullah alamoudi <[email protected]> Gerrit-HasComments: Yes
