Il ven 21 lug 2017, 20:11 Sijie Guo <guosi...@gmail.com> ha scritto: > Checked the code and also confirmed with Matteo: > > - if you are using V3 protocol, all the ByteBuf are unpooled because of the > deserialization of protobuf. It already makes the copy of the memory coming > from Netty. so there is not memory leak even you don't iterate over the > enumeration. > - if you are using V2 protocol, currently it will return the ByteBuf from > Netty to LedgerEntry (no memory copy there). Currently the Netty is > configured to using PooledByteBufAllocator.DEFAULT, so it is a pooled > bytebuf, you are responsible for releasing the buffer in LedgerEntry. > > > We can expose a configuration flag to use Pooled or Unpooled buffer > allocator in Netty. If it is configured as Unpooled, you don't need to > worry about releasing the buffer. If it is configured as Pooled, you hold > the responsibility to release the buffer when using V2 protocol. > > How does this sound? >
It sounds really good to me! I can work on it next week I would like to fix this for 4.5 > > > Other comments inline: > > On Fri, Jul 21, 2017 at 9:53 AM, Enrico Olivelli <eolive...@gmail.com> > wrote: > > > Il ven 21 lug 2017, 18:39 Sijie Guo <guosi...@gmail.com> ha scritto: > > > > > On Jul 21, 2017 9:25 AM, "Enrico Olivelli" <eolive...@gmail.com> > wrote: > > > > > > Il ven 21 lug 2017, 18:19 Sijie Guo <guosi...@gmail.com> ha scritto: > > > > > > > On Jul 21, 2017 12:59 AM, "Enrico Olivelli" <eolive...@gmail.com> > > wrote: > > > > > > > > Hi, > > > > I have just file this issue > > > > https://github.com/apache/bookkeeper/issues/271 > > > > > > > > After the switch to Netty 4 I noticed this "API change", that is very > > > > important to be documented and/or to be addresses. > > > > > > > > > > > > I think documentation is important in this case. We should make sure > it > > > > documented at the javadoc of this class. > > > > > > > > > > > > Inside LedgerEntry object we retain a ByteBuf, which is turn is > > > > automatically 'released' only in this cases: > > > > > > > > - getEntry() is called > > > > - getEntryInputStream() is called and the InputStream is closed > > > > - the ByteBuf is manually closed by the client > > > > > > > > The real tricky thing is that if the client calls readEntry and the > > > > Enumeration is not fully processed we are going to leak ByteBufs and > so > > > > head/direct memory. > > > > > > > > > > > > I am wondering when this will happen - if you already called read > > > entries, > > > > why you don't enumerate the entries. The first two approaches are > > > backward > > > > compatible with existing behavior without changing any codes. The > third > > > one > > > > is the new API that gave the client flexibility on managing the > memory > > > > usage itself. > > > > > > > > > > It can happen if you have an application error while processing the > > > entries and so you stop calling getEntry for the remaing part of the > > > enumeration. > > > > > > > > > If you stop calling get entries, because you application is closing > after > > > errors, or? > > > > > > > Maybe the application falls into a temporary error and then needs to > start > > replaying the log from a previous position. > > > > If the application just falls into a temporary error, you still have the > ability to release the retrieved entries, no? > > I am trying understand - this is an 'annoying' problem, or a really hard > problem - e.g. you will lost the references to the entries and you don't > get any chances to release them. > > > > > > > > Or maybe the thread that is reading the enumeration needs to stop for any > > reason but the other part of the application will continue to go on. > > I have some cases where in the application are working several > indipendent > > 'followers' and only one gets an error or simply need to stop beeing a > > follower and start a recovery procedure to become 'leader', in this case > it > > can happen that the processor stops as soon as possible without > processing > > all the entries. > > > > It is seems that methods of LedgerEntry can be called only once or we > will > > release the buf more then once. This is another problem > > > > Enrico, the behavior before netty 4 change is same. You can't call getEntry > twice. Whether we should change this behavior is a separate > backward-compatible topic. > I checked in 4.4 and I agree with you at 100% We can throw some kind of IllegalStateException to make it clear Thanks Enrico > > > > > Enrico > > > > > > > > > If that is the case, the application is closing, you don't even need to > > > care about the memory, no? > > > > > > > > > > > > > > > > > > > > > > > My proposal: > > > > introduce some "entry reference counting" and ensure that all > generated > > > > entries by a LedgerHandler are "disposed" on LedgerHandler.close() > and > > > make > > > > sure that when BookKeeper client is closed all of the LedgerHandlers > > > > process their own disposal procedure > > > > > > > > > > > > I am not sure if we need this. It seems to be over engineering to me. > > > > > > > > > > > > > > > > -- Enrico > > > > > > > -- > > > > > > > > > -- Enrico Olivelli > > > > > -- > > > > > > -- Enrico Olivelli > > > -- -- Enrico Olivelli
