Discuss the lifecycle of application data bytebuffer when adding new entries to ledger

[karanmehta93]

QUESTION

The issue here and the corresponding PR here made some deep changes as to how application level byte buffers are utilized and released by bookkeeper client.

With this change, application level data buffer should not be reused anytime and the complete lifecycle of managing the refcounts and release of memory should be left to bk client.
If byte buf is reused, it can potentially corrupt the entries (Qw - Qa), attributed to certain race conditions in the code, which usually surface only under high load. The commit hasn't reverted from the community. The commit offers reduction in GC cycles and CPU usage.

As discussed in slack group, we had some discussion along these lines.

With the BetyBuf (refcounted) API since 4.8 we stated clearly (and changed behavior) that the net effect of addEntry is a -1 on the refcount, in other words BK takes ownership of the ByteBuf and it is now the responsible for disposal.
In 4.7 it was not so clear

However concerns were raised regarding the usage of the API public long addEntry(byte[] data, int offset, int length), in which the application expects only part of buffer to be written to ledger. If bk client releases the buffer, the application can run into unexpected issues. If application reuses the buffer, it can lead to data corruption.

The issue is to discuss proper handling of these byte buffers and managing their lifecycle. We should standardize and put it up in the doc.

@eolivelli @sijie @jvrao @reddycharan @nicmichael Thoughts welcome.

[eolivelli]

I think that it is better to have a safe and robust API.
We should add a copy of the byte[].
If the user wants extra performances it can use the ByteBuf based API.

If you are using the (byte[], int, int) method it is very likely that you are using a part of a shared buffer and you are going to re use it, but with current behavior of BK you cannot write to the array anymore, because you won't ever know if BK is going to use it, due to retries.

I would exclude some ClientConfiguration flag for not doing the copy, we would need to duplicate many tests and it will be very messy from the user's point of view.

[eolivelli]

I am adding "priority/blocker" label because we are talking about data corruption and we must take a decision before the next release

[reddycharan]

in the past I raised similar concern regarding rules/expectations of refcount of bytebufs, which are passed to client API

#1237

@eolivelli @jvrao @sijie

[sijie]

regarding the reference count question, i don't think it is a problem whether the ref count starts with 0 or -1. If we clearly state how the reference count was handling when bookkeeper takes ByteBuf, then we are good. We agreed on adopting the netty's principle - the receiver is responsible for releasing the ByteBuf. That was adopted since 4.8.0.

regarding the question for byte[], I think there is a default principle in java world - if you are passing byte[] between methods or modules, try to avoid to share or reuse the byte[] array. In other words, if you pass in a byte[] to bookkeeper, you shouldn't mutate the byte[] array anymore. otherwise it can cause any data corruption.

I haven't followed closely on the question. I wasn't sure what was the problem. but regarding the question about public long addEntry(byte[] data, int offset, int length)

If bk client releases the buffer, the application can run into unexpected issues.

There is nothing to release for byte[] here from bookkeeper client's POV. I am not sure what does it mean here.

If application reuses the buffer, it can lead to data corruption.

Yes if you pass in a byte[] array, application shouldn't reuse the buffer.

---

@eolivelli

We should add a copy of the byte[].
If the user wants extra performances it can use the ByteBuf based API.

Why? The application should be do that for byte[]. It knows how to allocate the buffer.

but with current behavior of BK you cannot write to the array anymore, because you won't ever know if BK is going to use it, due to retries.

That should be a default behavior to most of the Java applications. Not just BK. A lot of IO interfaces in Java takes byte[] arrays. They expect the byte[] arrays are not be shared or reused.

[eolivelli]

@sijie
I think that usually in Java when you pass (byte[],int,int) reference to a method like OutputStream#write(byte[], int, int) you expect to be able to recycle that byte[] array after returning from that call or catching an exception.

In this case our blocking API is not following the same behavior, if you have AQ<WQ you must not use the byte[] anymore, or at least never write to it (this is @karanmehta93 case).

For non blocking (async) APIs it is less obvious how it should work but generally I would expect to be able to reuse my buffer as soon as the callback is invoked.

They expect the byte[] arrays are not be shared or reused.

Can you please give me an example in the JDK? I can't recall any example. Maybe I am only missing some point of view.

[eolivelli]

I think we shoul at least add explicit javadocs and let the application perform the copy if needed

[Ghatage]

@karanmehta93 Could you please update this discussion in light of your latest ByteBuf changes under #2441?

[karanmehta93]

The ByteBuf changes in #2441 are orthogonal to this issue. Buffers were managed completely by BK client before and after this change as well. #2441 is an internal optimization, which should be transparent to all users of BK.