Commit 5bd4e27cbc4dd800d88188c4b39f59447e02e44a in branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=5bd4e27 ]

ACCUMULO-2160 fixed up a lot of findbugs/pmd complaints

Commit 5bd4e27cbc4dd800d88188c4b39f59447e02e44a in branch refs/heads/master from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=5bd4e27 ]

ACCUMULO-2160 fixed up a lot of findbugs/pmd complaints

Do we really want PasswordToken to return a copy on getPassword()? Will that violate it's "Destroyable"-ness in any way (maybe when it's overridden)? I'm not sure. Just want to make sure we're not doing something we don't want to do.

I also wonder how many of these changes can be represented as warnings in Eclipse (or your IDE of choosing) and whether we should make them standards. Eclipse may even support some of these things (like stripping redundant modifiers off interfaces) as save-actions.

Also, I noticed that many changes here added trailing whitespace (including indented blank lines).

In TestLruBlockCache (perhaps elsewhere also), it should probably have used JUnit's Assert.assertArrayEquals(), for more informative failure messages, rather than assertTrue(Arrays.equals())

It's curious that Math.abs was inline'd into the class. What's the reasoning for that in the examples?

Oh, I probably shouldn't have inlined abs in the examples. It's too confusing. Random.nextLong() can return Long.MIN_VALUE. Math.abs(Long.MIN_VALUE) == Long.MIN_VALUE, which is still negative. I should at least make the function return Long.MAX_VALUE or zero or something.

I have tried to get the formatter to work for me, but for some reason it doesn't. I'll spend some time on that.

Unfortunately, there's no immutable array type. findbugs was not happy about keeping or returning refs to mutable arrays. PasswordToken is still Destroyable, but it may have leaked the password, but that was already possible.

Commit 2d51e329e1784ccf18bd089dcc7bb4c932909581 in branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=2d51e32 ]

ACCUMULO-2160 review fixes

Commit 2d51e329e1784ccf18bd089dcc7bb4c932909581 in branch refs/heads/master from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=2d51e32 ]

ACCUMULO-2160 review fixes

Unfortunately, there's no immutable array type. findbugs was not happy about keeping or returning refs to mutable arrays. PasswordToken is still Destroyable, but it may have leaked the password, but that was already possible.

Guava has some immutable array wrappers

Unfortunately, there's no immutable array type. findbugs was not happy about keeping or returning refs to mutable arrays. PasswordToken is still Destroyable, but it may have leaked the password, but that was already possible.

I believe the way to do this with base JDK components is a read-only ByteBuffer.

Commit 5dd6f84bc1858d894cfbaedbdecfb9acae15f877 in branch refs/heads/1.4.5-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=5dd6f84 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit 6d49e1a48af4dfb0b9d5a8bcf152567b97dd9c79 in branch refs/heads/1.4.5-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=6d49e1a ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit 5dd6f84bc1858d894cfbaedbdecfb9acae15f877 in branch refs/heads/1.5.1-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=5dd6f84 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit cb50a743dee006bcbe1bf196ff224af5557722f6 in branch refs/heads/1.5.1-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=cb50a74 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit 6d49e1a48af4dfb0b9d5a8bcf152567b97dd9c79 in branch refs/heads/1.5.1-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=6d49e1a ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit 5dd6f84bc1858d894cfbaedbdecfb9acae15f877 in branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=5dd6f84 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit cb50a743dee006bcbe1bf196ff224af5557722f6 in branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=cb50a74 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit e135e039a33c1e3917bed87c16efcf6535418d15 in branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=e135e03 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit 2af251552e93b41875cf9d97aa927a448d7d7b00 in branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=2af2515 ]

ACCUMULO-2160 back-port serious findbugs/pmd issues to 1.5/1.4

Commit cb50a743dee006bcbe1bf196ff224af5557722f6 in branch refs/heads/master from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=cb50a74 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit e135e039a33c1e3917bed87c16efcf6535418d15 in branch refs/heads/master from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=e135e03 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit e4db592367099b53c6880ca2c94a851963b76efc in branch refs/heads/master from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=e4db592 ]

ACCUMULO-2160 back-port real bugs found by findbugs

Commit 2af251552e93b41875cf9d97aa927a448d7d7b00 in branch refs/heads/master from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=2af2515 ]

ACCUMULO-2160 back-port serious findbugs/pmd issues to 1.5/1.4

Thanks for doing this. I'm a huge Findbugs fan. Some items that jumped out at me:

• In several places you switched out Math.random() for Random.nextInt(). It may be worthwhile creating a single Random instance for all code to use, so that there aren't a bunch of them around. The downside is that Random isn't thread-safe, so a shared instance would have to be synchronized. I'll leave it to you whether that is an issue for performance (Math.random() had to synchronize anyway).
• The change in the equals() methods of Condition and ConditionalMutation mean that their subclasses can now be considered equal to superinstances. That might matter, it might not.
• I saw some stubbed hashCode() methods, which makes sense for this ticket, but we'll need to fix those.
• The correction in Authorizations.contains(String) seems like it could be huge, since before it would always return false. Did that have any impact?
• The correction in TabletServer.AssignmentHandler.run() also seems like it could have some significance in making recentlyUnloadedCache work better - maybe causing more UNLOAD_FAILURE_NOT_SERVING messages to be generated. (Looks like a good thing.)

Math.random() for Random.nextInt()

I'm guessing that making new instances is cheap enough, but I don't know.

mean that their subclasses can now be considered equal to superinstances

Right, that was the findbugs complaint: that it wouldn't work for subclasses.

Authorizations.contains(String) seems like it could be huge

I'm not sure it changes the behavior at all.

making recentlyUnloadedCache work better

Yep. I ported it back to 1.4 and 1.5, too.

Right, that was the findbugs complaint: that it wouldn't work for subclasses.

Yeah, it's that whole thing about whether equals() should rely on getClass() or instanceof. I'm not judging which is better, and apparently Findbugs prefers instanceof; only that this is a change of some significance. I think the only effect currently is this, which as I said may or may not matter.

ConditionalWriterImpl.QCMutation qcm = something;  // extends ConditionalMutation
ConditionalMutation cm = (ConditionalMutation) something.clone();  // suppose clone works
assertTrue(cm.equals(qcm));  // used to be false
assertFalse(qcm.equals(cm));  // no symmetry

Commit 6d49e1a48af4dfb0b9d5a8bcf152567b97dd9c79 in branch refs/heads/1.6.0-SNAPSHOT from Eric Newton
[ https://git-wip-us.apache.org/repos/asf?p=accumulo.git;h=6d49e1a ]

ACCUMULO-2160 back-port real bugs found by findbugs

Reopen if there's something more you want to fix for 1.6.

FindBugs now runs in the build and automatically finds serious issues. (ACCUMULO-2697)