Flume
  1. Flume
  2. FLUME-1917

FileChannel group commit (coalesce fsync)

    Details

    • Type: New Feature New Feature
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: v1.4.0
    • Component/s: File Channel
    • Labels:
      None
    1. FLUME-1917.patch
      9 kB
      Hari Shreedharan

      Activity

      Hide
      Hudson added a comment -

      Integrated in flume-trunk #453 (See https://builds.apache.org/job/flume-trunk/453/)
      FLUME-1917: FileChannel group commit (coalesce fsync) (Revision b8c4b003a8273b17e8641a9e5bcafd2357fbd370)

      Result = FAILURE
      brock : http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=b8c4b003a8273b17e8641a9e5bcafd2357fbd370
      Files :

      • flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java
      • flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java
      • flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLogFile.java
      Show
      Hudson added a comment - Integrated in flume-trunk #453 (See https://builds.apache.org/job/flume-trunk/453/ ) FLUME-1917 : FileChannel group commit (coalesce fsync) (Revision b8c4b003a8273b17e8641a9e5bcafd2357fbd370) Result = FAILURE brock : http://git-wip-us.apache.org/repos/asf/flume/repo?p=flume.git&a=commit&h=b8c4b003a8273b17e8641a9e5bcafd2357fbd370 Files : flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Log.java flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/LogFile.java flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestLogFile.java
      Hide
      Mike Percy added a comment -

      Awesome!

      Show
      Mike Percy added a comment - Awesome!
      Hide
      Brock Noland added a comment -

      Thanks Hari! I committed this to trunk and 1.4.

      Show
      Brock Noland added a comment - Thanks Hari! I committed this to trunk and 1.4.
      Hide
      Brock Noland added a comment -

      Hey Hari,

      Yeah, I should have noted that thread-3 sync is a noop. The logic is obvious when we pass in the offset into the sync method but it looks like the current patch will result in the same number of syncs.

      Show
      Brock Noland added a comment - Hey Hari, Yeah, I should have noted that thread-3 sync is a noop. The logic is obvious when we pass in the offset into the sync method but it looks like the current patch will result in the same number of syncs.
      Hide
      Hari Shreedharan added a comment -

      Reading that comment again, I guess I could make it clearer. Step-by-step, with this patch:
      thread-1 commit - lastSyncOffset = 0, lastCommitOffset = t-1 cOffset
      thread-2 commit - lastSyncOffset = 0, lastCommitOffset = t-2 cOffset (t-2 cOffset > t-1 cOffset)
      thread-2 sync - lastSyncOffset = t-2 cOffset, lastCommitOffset = t-2 cOffset (fsync, since t-2 cOffset > 0)
      thread-3 commit - lastSyncOffset = t-2 cOffset, lastCommitOffset = t-3 cOffset (now t-3 cOffset > t-2 cOffset)
      thread-1 sync - lastSyncOffset = t-3 cOffset, lastCommitOffset = t-3 cOffset (fsync, since t-3 cOffset > t-2 cOffset)
      thread-3 sync - lastSyncOffset = t-3 cOffset, lastCommitOffset = t-3 cOffset (no-op, since t-3 cOffset == t-3 cOffset).

      So the idea of group commit works fine, just that the fsync happens in a different thread than the one that wrote to the last offset. I don't think that is an issue since the total number of fsyncs is 2 as expected.

      Show
      Hari Shreedharan added a comment - Reading that comment again, I guess I could make it clearer. Step-by-step, with this patch: thread-1 commit - lastSyncOffset = 0, lastCommitOffset = t-1 cOffset thread-2 commit - lastSyncOffset = 0, lastCommitOffset = t-2 cOffset (t-2 cOffset > t-1 cOffset) thread-2 sync - lastSyncOffset = t-2 cOffset, lastCommitOffset = t-2 cOffset (fsync, since t-2 cOffset > 0) thread-3 commit - lastSyncOffset = t-2 cOffset, lastCommitOffset = t-3 cOffset (now t-3 cOffset > t-2 cOffset) thread-1 sync - lastSyncOffset = t-3 cOffset, lastCommitOffset = t-3 cOffset (fsync, since t-3 cOffset > t-2 cOffset) thread-3 sync - lastSyncOffset = t-3 cOffset, lastCommitOffset = t-3 cOffset (no-op, since t-3 cOffset == t-3 cOffset). So the idea of group commit works fine, just that the fsync happens in a different thread than the one that wrote to the last offset. I don't think that is an issue since the total number of fsyncs is 2 as expected.
      Hide
      Hari Shreedharan added a comment -

      Brock,

      In your example, the thread-3 sync would end up being a no-op, right? Since the sync by thread-1 happened after thread-3 updated the lastCommitPosition. After that thread-3's sync has no effect because the lastSyncOffset is now the same as the lastCommitOffset(this was updated by thread-1 when the sync happened). In the scenario you describe above, minimum of 2 syncs are required, and it looks like only 2 would actually happen.

      Show
      Hari Shreedharan added a comment - Brock, In your example, the thread-3 sync would end up being a no-op, right? Since the sync by thread-1 happened after thread-3 updated the lastCommitPosition. After that thread-3's sync has no effect because the lastSyncOffset is now the same as the lastCommitOffset(this was updated by thread-1 when the sync happened). In the scenario you describe above, minimum of 2 syncs are required, and it looks like only 2 would actually happen.
      Hide
      Brock Noland added a comment -

      Hey Hari,

      Thanks for the patch! I think the patch will work, but I think there is a race that could cause "extra" syncs? Let me know if I am off base:

      thread-1 commit
      thread-2 commit
      thread-2 sync
      thread-3 commit
      thread-1 sync <- here thread-1 will do a sync if though it's required?
      thread-3 sync
      

      I think we could eliminate this by returning the offset from put/rollback and then passing into sync?

      Show
      Brock Noland added a comment - Hey Hari, Thanks for the patch! I think the patch will work, but I think there is a race that could cause "extra" syncs? Let me know if I am off base: thread-1 commit thread-2 commit thread-2 sync thread-3 commit thread-1 sync <- here thread-1 will do a sync if though it's required? thread-3 sync I think we could eliminate this by returning the offset from put/rollback and then passing into sync?
      Hide
      Hari Shreedharan added a comment -

      Moved the sync out of the commit calls and make those calls explicitly. sync() method will fsync only if the last commit position is higher than the last sync position.

      Show
      Hari Shreedharan added a comment - Moved the sync out of the commit calls and make those calls explicitly. sync() method will fsync only if the last commit position is higher than the last sync position.
      Hide
      Hari Shreedharan added a comment -

      Actually commit is already split up. I did exactly that - thought right now the commit happens from synchronized blocks, so really there is no concurrency in the commits/syncs (syncs are called from synchronized methods).

      Show
      Hari Shreedharan added a comment - Actually commit is already split up. I did exactly that - thought right now the commit happens from synchronized blocks, so really there is no concurrency in the commits/syncs (syncs are called from synchronized methods).
      Hide
      Brock Noland added a comment -

      Yeah it'd be interesting to see. Offhand was thinking we'd keep the synchronization and break LogFile.commit into two calls, commit and sync(offset). The commit method would return the offset of a commit and then the sync method would track the last offset a sync was called and if the last sync was greater than or equal to the offset the writer was interested in we wouldn't have to actually do the sync.

      Show
      Brock Noland added a comment - Yeah it'd be interesting to see. Offhand was thinking we'd keep the synchronization and break LogFile.commit into two calls, commit and sync(offset). The commit method would return the offset of a commit and then the sync method would track the last offset a sync was called and if the last sync was greater than or equal to the offset the writer was interested in we wouldn't have to actually do the sync.
      Hide
      Hari Shreedharan added a comment -

      Brock - the 2nd approach is what I implemented. I will do some refactoring to remove a buch of the synchronization. This is mostly for the rolling logic, so should be fairly simple handle.

      Show
      Hari Shreedharan added a comment - Brock - the 2nd approach is what I implemented. I will do some refactoring to remove a buch of the synchronization. This is mostly for the rolling logic, so should be fairly simple handle.
      Hide
      Brock Noland added a comment -

      I think this is going to require refactoring of code and also our approach to how to we commit. From my perspective, what we want to avoid is:

      thread-1: write event1
      thread-2: write event2
      thread-1: write event3
      thread-2: commit and sync
      thread-1: commit and sync (immediately after thread-2)

      That is we don't want multiple threads doing subsequent commits and then syncs. It'd be ideal if we had:

      thread-1: write event1
      thread-2: write event2
      thread-1: write event3
      thread-2: commit
      thread-1: commit
      thread-2: sync
      thread-1: sync – noop sync we already synced our commit.

      Show
      Brock Noland added a comment - I think this is going to require refactoring of code and also our approach to how to we commit. From my perspective, what we want to avoid is: thread-1: write event1 thread-2: write event2 thread-1: write event3 thread-2: commit and sync thread-1: commit and sync (immediately after thread-2) That is we don't want multiple threads doing subsequent commits and then syncs. It'd be ideal if we had: thread-1: write event1 thread-2: write event2 thread-1: write event3 thread-2: commit thread-1: commit thread-2: sync thread-1: sync – noop sync we already synced our commit.
      Hide
      Hari Shreedharan added a comment -

      Looks like all the methods that write to the file are synchronized, so not sure group commits can achieve much without considerable refactoring.

      Show
      Hari Shreedharan added a comment - Looks like all the methods that write to the file are synchronized, so not sure group commits can achieve much without considerable refactoring.
      Hide
      Mike Percy added a comment -

      Hope you guys don't mind I just updated the issue title

      Show
      Mike Percy added a comment - Hope you guys don't mind I just updated the issue title
      Hide
      Hari Shreedharan added a comment -

      Yep, that is correct. I am trying to implement a simple version of what is described in that post (sorry, I didnt post the link myself!)

      Show
      Hari Shreedharan added a comment - Yep, that is correct. I am trying to implement a simple version of what is described in that post (sorry, I didnt post the link myself!)
      Hide
      Brock Noland added a comment -
      Show
      Brock Noland added a comment - I think this means group commit https://kb.askmonty.org/en/group-commit-for-the-binary-log/
      Hide
      Denny Ye added a comment -

      hi Hari, what's mean of this bug?

      Show
      Denny Ye added a comment - hi Hari, what's mean of this bug?

        People

        • Assignee:
          Hari Shreedharan
          Reporter:
          Hari Shreedharan
        • Votes:
          0 Vote for this issue
          Watchers:
          5 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development