On Sat, May 6, 2017 at 9:51 AM, Charan Reddy G <reddychara...@gmail.com>
wrote:
> Hey,
>
> In EntryLogger, I'm wondering what is the need of making flushCurrentLog
> method synchronized? (if we make logChannel variable volatile or of type
> AtomicReference and bytesWrittenSinceLastFlush of type AtomicLong). Anyhow
> in BufferedChannel.flush, flushInternal() is called from synchrnoized block
> of BufferedChannel.
>
> EntryLogger.flushCurrentLog
> synchronized void flushCurrentLog() throws IOException {
> if (logChannel != null) {
> logChannel.flush(true);
> bytesWrittenSinceLastFlush = 0;
> LOG.debug("Flush and sync current entry logger {}.",
> logChannel.getLogId());
> }
> }
>
> BufferedChannel.flush
> public void flush(boolean shouldForceWrite) throws IOException {
> synchronized(this) {
> flushInternal();
> }
> if (shouldForceWrite) {
> forceWrite(false);
> }
> }
>
I don't think the synchronized is necessary here. Atomic structures can
help here.
>
>
> I'm currently working on "(BOOKKEEPER-1041) Multiple active entrylogs".
> For this feature I need to make changes to checkpoint logic. Currently with
> BOOKKEEPER-564 change, we are scheduling checkpoint only when current
> entrylog file is rotated. So we dont call 'flushCurrentLog' when we
> checkpoint. But for BOOKKEEPER-1041 feature, since there are going to be
> multiple active entrylogs, scheduling checkpoint when entrylog file is
> rotated, is not an option. So I need to call flushCurrentLogs when
> checkpoint is made for every 'flushinterval' period. Here it would be
> optimal if flushCurrentLog is not in synchronized block, also I don't see a
> reason for why it has to be in synchrnoized block to begin with. In
> BufferedChannel.flush method 'forceWrite' method (which is not in
> synchronized block of BufferedChannel) is gonna take considerable time,
> since it flush/forceWrites file to disk. So if EntryLogger's lock is not
> held during the forceWrite of BufferedChannel, then progress can be made in
> EntryLogger.addEntry.
>
Since you are going to change the checkpointing logic, can you explain more
in a separate email thread? I'd like to make sure the changes will not
cause any performance degradation.
>
> JV and I had gone through this code mutiple times and we couldn't find a
> reason for it to be synchrnoized. Please let us know if we are missing
> something here.
>
> Thanks,
> Charan
>
