accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 19703: WIP - Address 'high' priority findbugs results
Date Thu, 27 Mar 2014 13:27:53 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message