Uploaded image for project: 'Bookkeeper'
  1. Bookkeeper
  2. BOOKKEEPER-235

Bad syncing in entrylogger degrades performance for many concurrent ledgers

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1.0
    • Component/s: None
    • Labels:
      None

      Description

      EntryLogger flush syncs on the wrong object, which really hurts performance.

      1. BOOKKEEPER-235.diff
        2 kB
        Ivan Kelly
      2. BOOKKEEPER-235.diff
        1 kB
        Ivan Kelly

        Activity

        Hide
        ikelly Ivan Kelly added a comment -

        The problem was that the flush thread was syncing on entryLogger, while EntryLogger#addEntry is also synchronised, so they were holding each other off.

        Fix is two liner.

        Show
        ikelly Ivan Kelly added a comment - The problem was that the flush thread was syncing on entryLogger, while EntryLogger#addEntry is also synchronised, so they were holding each other off. Fix is two liner.
        Hide
        fpj Flavio Junqueira added a comment -

        The description refers to EntryLogger flush synchronizing on the wrong object, but the patch changes a synchronization block in InterleavedLedgerStore. For InterleavedLedgerStore flush, it is only called from Bookie SyncThread, and I'm not convinced that it is ok to change the synchronized block like that.

        Show
        fpj Flavio Junqueira added a comment - The description refers to EntryLogger flush synchronizing on the wrong object, but the patch changes a synchronization block in InterleavedLedgerStore. For InterleavedLedgerStore flush, it is only called from Bookie SyncThread, and I'm not convinced that it is ok to change the synchronized block like that.
        Hide
        ikelly Ivan Kelly added a comment -

        InterleavedLedgerStorage#flush was syncing on the entrylogger to ensure that only #flush was not called concurrently. It was something i introduced in BOOKKEEPER-196 as an extra check even though it was only called by SyncThread. I didn't realize at the time that EntryLogger#addEntry was also synchronized. Because of this, each call to #addEntry from the bookie thread would block the sync block of #flush in the sync thread.

        Show
        ikelly Ivan Kelly added a comment - InterleavedLedgerStorage#flush was syncing on the entrylogger to ensure that only #flush was not called concurrently. It was something i introduced in BOOKKEEPER-196 as an extra check even though it was only called by SyncThread. I didn't realize at the time that EntryLogger#addEntry was also synchronized. Because of this, each call to #addEntry from the bookie thread would block the sync block of #flush in the sync thread.
        Hide
        fpj Flavio Junqueira added a comment -

        InterleavedLedgerStorage#flush essentially calls LedgerCacheImpl#flushLedger and EntryLogger#flush. They both have synchronization in some form. Given that flushLock is kind of dangling in this patch, what if we simply remove the synchronization block in InterleavedLedgerStorage#flush and drop flushLock?

        Show
        fpj Flavio Junqueira added a comment - InterleavedLedgerStorage#flush essentially calls LedgerCacheImpl#flushLedger and EntryLogger#flush. They both have synchronization in some form. Given that flushLock is kind of dangling in this patch, what if we simply remove the synchronization block in InterleavedLedgerStorage#flush and drop flushLock?
        Hide
        fpj Flavio Junqueira added a comment -

        Canceling patch until we converge.

        Show
        fpj Flavio Junqueira added a comment - Canceling patch until we converge.
        Hide
        ikelly Ivan Kelly added a comment -

        New patch removes syncing completely, as a) this is only called from SyncThread and b) LedgerCacheImpl#flushLedger and EntryLogger#flush both have their own synchronisation.

        Show
        ikelly Ivan Kelly added a comment - New patch removes syncing completely, as a) this is only called from SyncThread and b) LedgerCacheImpl#flushLedger and EntryLogger#flush both have their own synchronisation.
        Hide
        fpj Flavio Junqueira added a comment -

        +1, thanks ivan.

        Show
        fpj Flavio Junqueira added a comment - +1, thanks ivan.
        Hide
        fpj Flavio Junqueira added a comment -

        Committed revision 1336083.

        Show
        fpj Flavio Junqueira added a comment - Committed revision 1336083.
        Hide
        hudson Hudson added a comment -

        Integrated in bookkeeper-trunk #499 (See https://builds.apache.org/job/bookkeeper-trunk/499/)
        BOOKKEEPER-235: Bad syncing in entrylogger degrades performance for many concurrent ledgers (ivank via fpj) (Revision 1336083)

        Result = UNSTABLE
        fpj :
        Files :

        • /zookeeper/bookkeeper/trunk/CHANGES.txt
        • /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java
        Show
        hudson Hudson added a comment - Integrated in bookkeeper-trunk #499 (See https://builds.apache.org/job/bookkeeper-trunk/499/ ) BOOKKEEPER-235 : Bad syncing in entrylogger degrades performance for many concurrent ledgers (ivank via fpj) (Revision 1336083) Result = UNSTABLE fpj : Files : /zookeeper/bookkeeper/trunk/CHANGES.txt /zookeeper/bookkeeper/trunk/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/InterleavedLedgerStorage.java

          People

          • Assignee:
            ikelly Ivan Kelly
            Reporter:
            ikelly Ivan Kelly
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development