[GitHub] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129337447 ## 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: @sijie I will update the patch as soon as possibile. Thank you 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] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129333806 ## 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: @sijie I don't want to change the behavior, I am fine with calling getEntry only once. I only wanted to make it more clear. When I and my colleague Diego saw actual code we got a little worried and we needed a double check with you and Matteo on the mailing list to be sure that there was no real problem. Anyway I am ok with reverting for the getEntry() as the IlegalStateException will be thrown in ByteBuf#release(). But for the InputStream case I think it will be too late to see the error at the 'close' of the inputstream. I will revert if you prefer, no problem 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] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248338 ## 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: ok, done 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] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248305 ## 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: done 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] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248134 ## 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 as above 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] eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs
eolivelli commented on a change in pull request #276: Issue-271 LedgerHandle#readEntries leaks ByteBufs URL: https://github.com/apache/bookkeeper/pull/276#discussion_r129248110 ## 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: @sijie the first time getEntry is called we will set data to null, this way the client cannot re-use the buffer anymore. On an eventual "second call" we will throw an IllegalStateException which explains the error. if I drop the 'if' the user will get a NullPointerException, but this is not very clear and IMHO it is bad practice. Unfortunately the ByteBuf can be used only once, because we have no clue to reset the original start position for the entry 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