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? Other comments inline: On Fri, Jul 21, 2017 at 9:53 AM, Enrico Olivelli <[email protected]> wrote: > Il ven 21 lug 2017, 18:39 Sijie Guo <[email protected]> ha scritto: > > > On Jul 21, 2017 9:25 AM, "Enrico Olivelli" <[email protected]> wrote: > > > > Il ven 21 lug 2017, 18:19 Sijie Guo <[email protected]> ha scritto: > > > > > On Jul 21, 2017 12:59 AM, "Enrico Olivelli" <[email protected]> > 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. > > 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 >
