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.
⨠throw new IllegalStateException("Unable to execute " + script
+ ". Stderr is: " + stderr + "\nStdout is: " + stdout);
⨠^â¨
(see http://errorprone.info/bugpattern/Finally)â¨Note: Some input
files use unchecked or unsafe operations.â¨Note: 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
