You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Sijie Guo <gu...@gmail.com> on 2012/04/28 11:31:24 UTC
Re: Review Request: BOOKKEEPER-224 Fix findbugs in bookkeeper-server
component
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4873/#review7344
-----------------------------------------------------------
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/NIOServerFactory.java
<https://reviews.apache.org/r/4873/#comment16221>
why changing long to int?
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java
<https://reviews.apache.org/r/4873/#comment16222>
I think this code for common case that we don't need to acquire lock, which is for performance. If you removed it, each time we need to acquire the lock. it doesn't make sense.
- Sijie
On 2012-04-25 13:35:19, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4873/
> -----------------------------------------------------------
>
> (Updated 2012-04-25 13:35:19)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> Fix findbugs in bookkeeper-server component
>
>
> This addresses bug BOOKKEEPER-224.
> https://issues.apache.org/jira/browse/BOOKKEEPER-224
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 947ba6a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java 714f70f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 97e01c0
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java e737055
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 269581a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java 65d6947
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCacheImpl.java eb8a89e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerEntryPage.java 294684a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java b9cd624
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/DigestManager.java a5bb426
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java f0a165f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java c5687ae
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java 950fba2
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java a564536
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 3c225c6
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java 8fd7df8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerLayout.java 3a3a81e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java b62ad3a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java f02d838
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/NIOServerFactory.java 3dfb67d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 2ee9cb4
> bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/BookKeeperTools.java e5eea00
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/HardLink.java 6e6c3e7
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 1d47643
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/NIOServerFactoryTest.java e2b2758
>
> Diff: https://reviews.apache.org/r/4873/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>
Re: Review Request: BOOKKEEPER-224 Fix findbugs in bookkeeper-server
component
Posted by Ivan Kelly <iv...@apache.org>.
> On 2012-04-28 09:31:24, Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/NIOServerFactory.java, line 493
> > <https://reviews.apache.org/r/4873/diff/1/?file=104226#file104226line493>
> >
> > why changing long to int?
ah, I don't know why I changed that. I think i removed packetsReceived from the CnxnStats subclass first but then realised it was the one in NIOServerFactory which was the problem, so copied that one down. Well spotted.
> On 2012-04-28 09:31:24, Sijie Guo wrote:
> > bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java, line 177
> > <https://reviews.apache.org/r/4873/diff/1/?file=104227#file104227line177>
> >
> > I think this code for common case that we don't need to acquire lock, which is for performance. If you removed it, each time we need to acquire the lock. it doesn't make sense.
What was there wasn't correct.
It's covered here: http://en.wikipedia.org/wiki/Double-checked_locking
However, it's probably better to fix it with "volatile".
- Ivan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4873/#review7344
-----------------------------------------------------------
On 2012-04-25 13:35:19, Ivan Kelly wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4873/
> -----------------------------------------------------------
>
> (Updated 2012-04-25 13:35:19)
>
>
> Review request for bookkeeper.
>
>
> Summary
> -------
>
> Fix findbugs in bookkeeper-server component
>
>
> This addresses bug BOOKKEEPER-224.
> https://issues.apache.org/jira/browse/BOOKKEEPER-224
>
>
> Diffs
> -----
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Bookie.java 947ba6a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieException.java 714f70f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java 97e01c0
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileSystemUpgrade.java e737055
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java 269581a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java 65d6947
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerCacheImpl.java eb8a89e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/LedgerEntryPage.java 294684a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java b9cd624
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/DigestManager.java a5bb426
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java f0a165f
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java c5687ae
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java 950fba2
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java a564536
> bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java 3c225c6
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java 8fd7df8
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerLayout.java 3a3a81e
> bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java b62ad3a
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java f02d838
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/NIOServerFactory.java 3dfb67d
> bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/PerChannelBookieClient.java 2ee9cb4
> bookkeeper-server/src/main/java/org/apache/bookkeeper/tools/BookKeeperTools.java e5eea00
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/HardLink.java 6e6c3e7
> bookkeeper-server/src/main/java/org/apache/bookkeeper/util/LocalBookKeeper.java 1d47643
> bookkeeper-server/src/test/java/org/apache/bookkeeper/test/NIOServerFactoryTest.java e2b2758
>
> Diff: https://reviews.apache.org/r/4873/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ivan
>
>