Michael Blow has posted comments on this change. Change subject: Modularize feed adaptors ......................................................................
Patch Set 4: (7 comments) https://asterix-gerrit.ics.uci.edu/#/c/1430/4/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/HDFSDataSourceFactory.java File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/input/HDFSDataSourceFactory.java: Line 235: return Collections.unmodifiableList(Arrays.asList(recordReaderNames)); Comments which apply to all of the impls: - Should we just store the collection instead of the array, to avoid rebuilding it each time? - Make it static final? - Should this be a Set<> instead of a List<>? Seems these are unique, if order matters, could use SortedSet<>? https://asterix-gerrit.ics.uci.edu/#/c/1430/4/asterixdb/asterix-villain/pom.xml File asterixdb/asterix-villain/pom.xml: Line 84: <version>0.8.9-SNAPSHOT</version> Use ${project.version} here (same as other 0.8.9-SNAPSHOTs below) Line 126: <version>0.2.18-SNAPSHOT</version> Use ${hyracks.version} here (same as other 0.2.18-SNAPSHOTs below) Line 141: <version>1.2.17</version> I believe this version is already defined in depdendencyManagement, and should not be redefined here. Please check the other deps with versions for the same. (Not sure about Eclipse, but IDEA indicates in the IDE for this pom when the definition is being overridden) https://asterix-gerrit.ics.uci.edu/#/c/1430/4/asterixdb/asterix-villain/src/main/resources/META-INF/services/org.apache.asterix.external.api.IDataParserFactory File asterixdb/asterix-villain/src/main/resources/META-INF/services/org.apache.asterix.external.api.IDataParserFactory: Line 2: org.apache.asterix.villain.external.parser.rss.RSSParserFactory These are both nice to make modular, but probably should each be in their own module, not combined into one. https://asterix-gerrit.ics.uci.edu/#/c/1430/4/asterixdb/asterix-villain/src/main/resources/META-INF/services/org.apache.asterix.external.api.IRecordReaderFactory File asterixdb/asterix-villain/src/main/resources/META-INF/services/org.apache.asterix.external.api.IRecordReaderFactory: Line 2: org.apache.asterix.villain.external.reader.rss.RSSRecordReaderFactory These are both nice to make modular, but probably should each be in their own module, not combined into one. https://asterix-gerrit.ics.uci.edu/#/c/1430/4/asterixdb/asterix-villain/src/test/resources/runtimets/testsuite.xml File asterixdb/asterix-villain/src/test/resources/runtimets/testsuite.xml: Line 26: <!ENTITY TemporalQueries SYSTEM "queries/temporal/TemporalQueries.xml"> All of these entity decls are unused here and should be removed. -- To view, visit https://asterix-gerrit.ics.uci.edu/1430 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic4f95255f5493a813ee1f875b63a62e74bc47602 Gerrit-PatchSet: 4 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Xikui Wang <xkk...@gmail.com> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: Xikui Wang <xkk...@gmail.com> Gerrit-HasComments: Yes
