Details

    • Type: Task Task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.6.0
    • Component/s: None
    • Labels:

      Issue Links

        Activity

        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        Christopher Tubbs added a comment -

        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?

        Show
        Christopher Tubbs added a comment - 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?
        Hide
        Eric Newton added a comment -

        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.

        Show
        Eric Newton added a comment - 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.
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        John Vines added a comment -

        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

        Show
        John Vines added a comment - 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
        Hide
        Sean Busbey added a comment -

        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.

        Show
        Sean Busbey added a comment - 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.
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        Bill Havanki added a comment -

        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.)
        Show
        Bill Havanki added a comment - 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.)
        Hide
        Eric Newton added a comment -

        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.

        Show
        Eric Newton added a comment - 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.
        Hide
        Bill Havanki added a comment -

        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
        
        Show
        Bill Havanki added a comment - 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
        Hide
        ASF subversion and git services added a comment -

        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

        Show
        ASF subversion and git services added a comment - 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
        Hide
        Eric Newton added a comment -

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

        Show
        Eric Newton added a comment - Reopen if there's something more you want to fix for 1.6.
        Hide
        Christopher Tubbs added a comment -

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

        Show
        Christopher Tubbs added a comment - FindBugs now runs in the build and automatically finds serious issues. ( ACCUMULO-2697 )

          People

          • Assignee:
            Eric Newton
            Reporter:
            Eric Newton
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development