First off, I would absolutely love to see the warnings reduced and the quality of our code improved. I'm in favor of all four of the points, and I think it's a good start towards weeding out a lot of issues.
So regarding question 1: That awkwardness comes about because org.simple.json is all untyped Maps under the hood ( https://github.com/fangyidong/json-simple/blob/master/src/main/java/org/json/simple/JSONObject.java#L19). So anything that touches those classes starts requiring casts everywhere. A couple months ago a PR was made against the library that seems to have added a lot of this stuff ( https://github.com/fangyidong/json-simple/pull/113). Basically, it was forked (https://github.com/cliftonlabs/json-simple and https://cliftonlabs.github.io/json-simple/) and they tried to contribute it back and it's been sitting there. I strongly think we should consider swapping over to the fork. We'd have to do a bit of research, but it seems the intent was to be completely backwards compatible while updating things like generics and some various issues they presumably saw with the original. For 2, and I'm not an expert in this by any means, but I would be in favor of defaulting to UTF-8. It's most likely what's happening under the hood anyway (even though it's technically platform dependent), so defaulting probably makes it both more explicit and removes potential for issues if a given system has a different underlying default. Justin On Fri, Apr 21, 2017 at 8:50 AM, JJ Meyer <jjmey...@gmail.com> wrote: > Hello everybody, > > Currently our build has a significant amount of warnings (most from the > error prone plugin). A lot of this has been documented here: > https://issues.apache.org/jira/browse/METRON-617 > > I want to continue to work on this Jira. I really want to reduce the number > of warnings in our build. As the Jira points out, a lot of them are > unchecked casts or the implicit use of default charset. > > When starting this, I want to start with a specific module. I plan on > starting with `metron-interface` as it's fairly self contained and is > pretty new. Below I want to layout what I plan on doing. Please let me know > what you all think: > > 1. Suppress warnings where generics are not supported or checking type is > not possible. > 2. For all unit tests that require supplying a charset I'll supply the > UTF-8 charset. > 3. Update the API to have a charset configuration for each resource that > needs one. > 4. Remove @SuppressWarnings("ALL") on all unit tests. I found out error > prone doesn't support this level of suppression. Which is probably a good > thing. We will need to explicitly state what we want to suppress instead. > > Two big questions came up right away when I was thinking about the above: > > 1. When suppressing warnings. I see we sometimes cast a JSONObject to > Map<String, Object>. I know it extends Map, but is it really safe to do > something like the following? Should we have a utility that truly builds a > map that uses generics? I plan on doing some testing around this, but if > anyone has any experience with this it would be greatly appreciated. Here > is an example of what I am talking about: > > JSONObject message = ...; > @SuppressWarnings("unchecked") > Map<String, Object> state = (Map<String, Object>) message; > > > 2. This one is about configuring charset (#3 above). Specifically in > metron-rest. Right now, I believe there are 3 sensor resources (index, > enrichment, and parser) that throw warnings due to implicitly using the > default charset. I propose that we have 3 configs (listed below). These > configs would take any valid charset, default, or not set. If not set then > UTF-8 would be default. Does this seem fair? > > sensor: > index.encoding: UTF-8 > enrichment.encoding: UTF-8 > parser.encoding: UTF-8 > > > Does anyone see any problems that may occur if we go about it this way? Any > help on this would be very much appreciated. > > Thanks, > JJ >
