You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Charan Reddy G <re...@gmail.com> on 2017/05/06 01:51:49 UTC
Question regarding synchronization in EntryLogger
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'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.
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
Re: Question regarding synchronization in EntryLogger
Posted by Venkateswara Rao Jujjuri <ju...@gmail.com>.
Sure, Charan please have a small writeup so we can discus in next Thursday.
On Tue, May 9, 2017 at 10:27 PM, Enrico Olivelli <eo...@gmail.com>
wrote:
>
>
> Il mer 10 mag 2017, 01:59 Sijie Guo <gu...@gmail.com> ha scritto:
>
>> On Sat, May 6, 2017 at 9:51 AM, Charan Reddy G <re...@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.
>>
>
> Maybe a BookKeeper Proposal will be useful?
>
>>
>>
>> >
>> > 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
>> >
>>
> --
>
>
> -- Enrico Olivelli
>
--
Jvrao
---
First they ignore you, then they laugh at you, then they fight you, then
you win. - Mahatma Gandhi
Re: Question regarding synchronization in EntryLogger
Posted by Enrico Olivelli <eo...@gmail.com>.
Il mer 10 mag 2017, 01:59 Sijie Guo <gu...@gmail.com> ha scritto:
> On Sat, May 6, 2017 at 9:51 AM, Charan Reddy G <re...@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.
>
Maybe a BookKeeper Proposal will be useful?
>
>
> >
> > 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
> >
>
--
-- Enrico Olivelli
Re: Question regarding synchronization in EntryLogger
Posted by Sijie Guo <gu...@gmail.com>.
On Sat, May 6, 2017 at 9:51 AM, Charan Reddy G <re...@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
>