You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Mike Drob <md...@mdrob.com> on 2014/03/26 23:24:23 UTC
Review Request 19703: WIP - Address 'high' priority findbugs results
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19703/
-----------------------------------------------------------
Review request for accumulo and Josh Elser.
Repository: accumulo
Description
-------
Address 'high' priority findbugs results.
I ignored the majority of the default Charset warnings, but can go back and fix them if folks decide that we should worry about them.
I ignored all of the name shadowing conflicts, because those only occured when we moved classes between packages and had one extend the other for backwards compatibility. Those should maybe go away in a version or two.
I left several TODOs for places where I either couldn't figure out exactly what to do, or figured that the work would be too extensive so I wanted some community backing before proceeding.
As the summary states, this is a Work In Progress.
Diffs
-----
core/src/main/java/org/apache/accumulo/core/Constants.java e0e88eb65c985123507d68cfd8d2440b4216648a
core/src/main/java/org/apache/accumulo/core/client/admin/NamespaceOperationsImpl.java 569a3b6b92d985e71cafdaee7b0cf6dbad6aa792
core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java c550f153cda75af328f829e287ad98509fed33d0
core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java 16f22e479188e81f1aea4a2f17a34b4d14d7c96e
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/ClassSize.java 8abf425bdbdc4282255353304788f970a9597d09
core/src/main/java/org/apache/accumulo/core/iterators/user/AgeOffFilter.java 6e9a571441b489670a93f7f6a3f4db06375bb782
core/src/main/java/org/apache/accumulo/core/util/shell/commands/ConfigCommand.java c76a51fbca314a3fe0c8d3b0145eb54cd3a22030
core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java 2394063902c9dc86f56a1b184bb0d033ed857739
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java 009988e355556d784fa9eaabd0846c0dd862cdab
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/ProcessReference.java 9aa2449dce2658f9d223b88143acd298a31df07e
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java 2ef438ffdce65fe5504f40578a85a935564cfe3f
server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java f8b1702792bc8d05d94f57f1e071f386cc610fa4
server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java e40368779d0ca56ef780f1f7d2466da370b95595
trace/src/main/java/org/apache/accumulo/trace/instrument/TraceRunnable.java 41c765d253d8fc22e9ca62bd05c87882af20ab62
Diff: https://reviews.apache.org/r/19703/diff/
Testing
-------
Unit tests, so far.
Thanks,
Mike Drob
Re: Review Request 19703: WIP - Address 'high' priority findbugs results
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19703/#review38726
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/Constants.java
<https://reviews.apache.org/r/19703/#comment71001>
The list here is still mutable; "Changes to the returned list 'write through' to the array." (Java API) You can wrap the list using Collections.unmodifiableList to really lock it down.
core/src/main/java/org/apache/accumulo/core/iterators/user/AgeOffFilter.java
<https://reviews.apache.org/r/19703/#comment71002>
I think threshold should be initialized to zero to mimic the prior behavior in the case where a user does not call init(). (Though they should.)
The old code is strange in setting it to -1 before attempting to parse the TTL, almost as if -1 should be set if the parse throws an exception ... but that would stop the init() call. So perhaps -1 should properly be the default? Weird.
server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java
<https://reviews.apache.org/r/19703/#comment71003>
Superficially this looks like a spot where UUID.randomUUID() would be helpful. Just something to consider, maybe for the future.
trace/src/main/java/org/apache/accumulo/trace/instrument/TraceRunnable.java
<https://reviews.apache.org/r/19703/#comment71005>
Yeah, so:
- Runnable doesn't implement Comparable, so the cast may fail.
- The runnable field might be null.
- Bill Havanki
On March 26, 2014, 6:24 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19703/
> -----------------------------------------------------------
>
> (Updated March 26, 2014, 6:24 p.m.)
>
>
> Review request for accumulo and Josh Elser.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Address 'high' priority findbugs results.
>
> I ignored the majority of the default Charset warnings, but can go back and fix them if folks decide that we should worry about them.
> I ignored all of the name shadowing conflicts, because those only occured when we moved classes between packages and had one extend the other for backwards compatibility. Those should maybe go away in a version or two.
>
> I left several TODOs for places where I either couldn't figure out exactly what to do, or figured that the work would be too extensive so I wanted some community backing before proceeding.
>
> As the summary states, this is a Work In Progress.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/Constants.java e0e88eb65c985123507d68cfd8d2440b4216648a
> core/src/main/java/org/apache/accumulo/core/client/admin/NamespaceOperationsImpl.java 569a3b6b92d985e71cafdaee7b0cf6dbad6aa792
> core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocatorImpl.java c550f153cda75af328f829e287ad98509fed33d0
> core/src/main/java/org/apache/accumulo/core/conf/PropertyType.java 16f22e479188e81f1aea4a2f17a34b4d14d7c96e
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/ClassSize.java 8abf425bdbdc4282255353304788f970a9597d09
> core/src/main/java/org/apache/accumulo/core/iterators/user/AgeOffFilter.java 6e9a571441b489670a93f7f6a3f4db06375bb782
> core/src/main/java/org/apache/accumulo/core/util/shell/commands/ConfigCommand.java c76a51fbca314a3fe0c8d3b0145eb54cd3a22030
> core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java 2394063902c9dc86f56a1b184bb0d033ed857739
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java 009988e355556d784fa9eaabd0846c0dd862cdab
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/ProcessReference.java 9aa2449dce2658f9d223b88143acd298a31df07e
> server/base/src/main/java/org/apache/accumulo/server/fs/VolumeUtil.java 2ef438ffdce65fe5504f40578a85a935564cfe3f
> server/base/src/main/java/org/apache/accumulo/server/security/SecurityOperation.java f8b1702792bc8d05d94f57f1e071f386cc610fa4
> server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/OperationServlet.java e40368779d0ca56ef780f1f7d2466da370b95595
> trace/src/main/java/org/apache/accumulo/trace/instrument/TraceRunnable.java 41c765d253d8fc22e9ca62bd05c87882af20ab62
>
> Diff: https://reviews.apache.org/r/19703/diff/
>
>
> Testing
> -------
>
> Unit tests, so far.
>
>
> Thanks,
>
> Mike Drob
>
>