asterixdb-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Till Westmann (Code Review)" <do-not-re...@asterixdb.incubator.apache.org>
Subject Change in asterixdb[master]: WIP - Load parser from classpath in ParserFactoryProvider
Date Thu, 05 Jan 2017 06:54:02 GMT
Till Westmann has posted comments on this change.

Change subject: WIP - Load parser from classpath in ParserFactoryProvider
......................................................................


Patch Set 2:

(1 comment)

This looks pretty good (and I didn't know about the ServiceLoader - seems the be quite similar
indeed :) ).

Wrt to the split-up I think that we should have one resource file per jar. So for the current
packaging that has all the parsers in the same jar, having all the class names in a single
file seems to be the right approach.
I think that we should actually put the individual parsers into separate jars (by moving them
to maven submodules), but I think that that should be part of a different change.

This chage looks good, but it would be nice to take a look at the JSON imports in ParserFactoryProvider
before submitting. You could try to use the ServiceLoader instead of our own implementation,
as the ServiceLoader is probably better tested. But we can also leave that part as-is.

https://asterix-gerrit.ics.uci.edu/#/c/1416/2/asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/provider/ParserFactoryProvider.java
File asterixdb/asterix-external-data/src/main/java/org/apache/asterix/external/provider/ParserFactoryProvider.java:

Line 44: import org.codehaus.jettison.json.JSONObject;
I don't see a use for these JSON classes in this file, but maybe I've missed it. In an case
we are in the process of moving all of our JSON processing to Jackson (2.8.4) - that's in
another change. So if there are JSON processing needs here, we should consider using Jackson
as well.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2ac039fe3daaf0636cf004289bd0c8a3229197a9
Gerrit-PatchSet: 2
Gerrit-Project: asterixdb
Gerrit-Branch: master
Gerrit-Owner: Xikui Wang <xkkwww@gmail.com>
Gerrit-Reviewer: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Gerrit-Reviewer: Till Westmann <tillw@apache.org>
Gerrit-Reviewer: Xikui Wang <xkkwww@gmail.com>
Gerrit-HasComments: Yes

Mime
View raw message