logging-flume icon indicating copy to clipboard operation
logging-flume copied to clipboard

FLUME-3205:Remove unnecessary 'synchronized' in ResettableFileInputStream

Open tcsdn opened this issue 8 years ago • 4 comments

I found no concurrent access to ResettableFileInputStream object in flume code. Remove 'synchronized' will improve performance of SpoolDirectorySource. Tested with a file of 200MB,with 'synchronized' it took 9 seconds to read the file. Without 'synchronized' it took 7 seconds.

tcsdn avatar Dec 26 '17 15:12 tcsdn

@harishreedharan Can you help me review the change?

tcsdn avatar Dec 26 '17 15:12 tcsdn

Hi @Guangxian ,

Thank you for the PR!

I've taken a look at the change and it seems to be a nice catch!

Even though there might be no concurrent access to the method in the Flume codebase, there might be access "from the outside". Thus, it might be a good idea to make this change in a backwards compatible way. (Ie. extract the "contents" of the method and leave a synchronized read that calls the non-synchronized, new one or something like that.)

Please, let me know what you think.

Thank you,

Donat

bessbd avatar Dec 27 '17 10:12 bessbd

@bessbd Thank you for review. I add a NonSyncResettableFileInputStream class. Let ResettableFileInputStream extends NonSyncResettableFileInputStream. Please review it.

tcsdn avatar Dec 27 '17 11:12 tcsdn

Can one of the admins verify this patch?

asfgit avatar Aug 17 '18 13:08 asfgit