sijie commented on a change in pull request #276: Issue-271
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129331061
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerEntry.java
##########
@@ -53,22 +54,49 @@ public long getLength() {
return length;
}
+ /**
+ * Returns the content of the entry.
+ * This method can be called only once.
+ * While using v2 wire protocol this method will automatically release the
internal ByteBuf
+ * @return
+ */
public byte[] getEntry() {
+ if (data == null) {
Review comment:
@eolivelli
there are two separate questions here.
1) are you going to change this behavior to support multiple gets?
2) if you are not going to change this behavior, why do we need you change?
The first question is more about whether to keep the behavior same as
before. The behavior without your code change is exactly same as before. If you
want to change this behavior, you can mark the read position, read the data and
reset the read position. but your change doesn't seem to be this case.
for 2) change, without your change, it still throws IlegalStateException
because the bytebuf will be released twice. so I don't see a strong reason why
do we need your change here. I am not talking about the 'if (data == null)'
case, I am talking about the change in general. Because I do not see a strong
reason to change this if we are not changing the behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
