[GitHub] incubator-metron issue #389: METRON-612: Clean up Error Prone generated warn...

[justinleet <gi...@git.apache.org>]

Github user justinleet commented on the issue:

    https://github.com/apache/incubator-metron/pull/389
  
    The number of files looks a lot more intimidating than the PR actually is (Most is adding `@Override` rather than actual changes), but it can be split out if we want it to be.
    
    Besides actual fixes, http://errorprone.info/bugpattern/DefaultCharset warnings are very common and I don't touch them at all.  My guess is that we'd be good with UTF-8, but it's large enough to be it's own separate discussion. Assuming people agree, I'll create a separate ticket.
    
    Highlights.  In addition, a couple things that were in the neighborhood got touched.
    
    - `@Override` added throughout. A couple can't be, because they're in generated code. http://errorprone.info/bugpattern/MissingOverride
    - `QueueHandler.java` get a type on the internal Queue.  Just happened to catch it going be.  Nothing changes externally, just carries over the ZKQueue type.
    - A couple classes (e.g. `ContainerTracker` and `PersistentAccessTracker`) have fields changed to final for synchronization. http://errorprone.info/bugpattern/SynchronizeOnNonFinalField
    - `SimpleFieldTransformation` fixed logic.  Don't remember which Error Prone warning this was, because the answer was to remove the chunk of offending code entirely.
    - Lot of things are unboxed (e.g. Boolean to boolean). http://errorprone.info/bugpattern/BoxedPrimitiveConstructor
    - Couple of inner classes are made static because they don't depend on the enclosing class at all. http://errorprone.info/bugpattern/ClassCanBeStatic
    - `newInstance()` generally changed to `getConstructor().newInstance()`.  Added catches where appropriate. http://errorprone.info/bugpattern/ClassNewInstance
    - `FluxTopologyComponent` has try-finally rewritten to use try with resources. Integration tests still work. http://errorprone.info/bugpattern/Finally
    
    There's a couple miscellaneous leftover things, most notably:
    ```
    /Users/jleet/Documents/workspace/incubator-metron/metron-analytics/metron-maas-service/src/main/java/org/apache/metron/maas/service/runner/Runner.java:265: warning: [Finally] If you return or throw from a finally, then values returned or thrown from the try-catch block will be ignored. Consider using try-with-resources instead.
    \u2028          throw new IllegalStateException("Unable to execute " + script + ".  Stderr is: " + stderr + "\nStdout is: " + stdout);
    \u2028          ^\u2028
        (see http://errorprone.info/bugpattern/Finally)\u2028Note: Some input files use unchecked or unsafe operations.\u2028Note: Recompile with -Xlint:unchecked for details.
    ```
    I wasn't sure enough of what was going on, so I punted, given how large the PR already is.
    
    Along with this, there are a couple instances of http://errorprone.info/bugpattern/TypeParameterUnusedInFormals in helper code that I'm not terribly worried about, but we might want to take a look at (and possibly explicitly suppress)
    
    Testing for this was making sure all the unit and integration tests still work (given that most changes are things like `@Override` that don't have actual execution effect) + ensuring things can still flow through topologies correctly.  The largest change is is the `FluxTopologyComponent`, whose usage is in integration tests and should be covered by that.
    
    In addition to this, we could probably upgrade several of these to errors if we wanted to, specifically SynchronizeOnNonFinalField, BoxedPrimitiveConstructor, ClassCanBeStatic, and ClassNewInstance I believe are all gone.  Unfortunately, because of the generated code MissingOverride is here to stay.  I could do that in either this ticket if we're interested or in a follow-on.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---