[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
sijie commented on a change in pull request #276: Issue-271
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129336829
##
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 you don't have to revert it. it is good to make it clear. I just
want to clarify to make things clear at this pull request. for assertion, I
would prefer using guava Preconditions.checkState(...).
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129106589 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java ## @@ -114,6 +116,10 @@ protected final static String ENABLE_BOOKIE_FAILURE_TRACKING = "enableBookieFailureTracking"; protected final static String BOOKIE_FAILURE_HISTORY_EXPIRATION_MS = "bookieFailureHistoryExpirationMSec"; +// Netty Review comment: can you move this parameter to section ' // NIO Parameters '? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
sijie commented on a change in pull request #276: Issue-271
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129105945
##
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
Review comment:
if you are adding javadoc, please follow the new checkstyle rules.
`
Returns ...
This method can ...
While using ...
@return
`
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
sijie commented on a change in pull request #276: Issue-271
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129106352
##
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) {
+throw new IllegalStateException("entry content can be accessed
only once");
+}
byte[] entry = new byte[data.readableBytes()];
data.readBytes(entry);
data.release();
+data = null;
return entry;
}
+/**
+ * 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 when calling the close
+ * method of the returned InputStream
+ *
+ * @return an InputStream which gives access to the content of the entry
+ */
public InputStream getEntryInputStream() {
-return new ByteBufInputStream(data);
+if (data == null) {
Review comment:
same question: it doesn't seem to be necessary?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
sijie commented on a change in pull request #276: Issue-271
LedgerHandle#readEntries leaks ByteBufs
URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129106275
##
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:
I don't understand here:
why do you need this change here? it doesn't seem to be necessary.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
sijie commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129106853 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ClientConfiguration.java ## @@ -114,6 +116,10 @@ protected final static String ENABLE_BOOKIE_FAILURE_TRACKING = "enableBookieFailureTracking"; protected final static String BOOKIE_FAILURE_HISTORY_EXPIRATION_MS = "bookieFailureHistoryExpirationMSec"; +// Netty +protected final static String NETTY_USE_POOLED_BUFFERS = "nettyUsePooledBuffers"; +protected final static boolean DEFAULT_NETTY_USE_POOLED_BUFFERS = true; Review comment: it is not a common practice to have the default value here. I'd prefer stick to current practice. If we are going to change the practice, let's change them all. 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: us...@infra.apache.org With regards, Apache Git Services
