ivankelly commented on a change in pull request #727: Issue 693: add interface and implementation of LedgerEntries URL: https://github.com/apache/bookkeeper/pull/727#discussion_r152675436
########## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/api/LedgerEntries.java ########## @@ -35,10 +35,15 @@ LedgerEntry getEntry(long entryId); /** - * In this, It does not retain the ByteBuf references for the entries in this LedgerEntries. + * In this method, It does not increment the reference counts of ByteBuf for the entries in this LedgerEntries. * The caller who calls {@link #iterator()} should be careful for not releasing the references. * - * @return the iterator of type LedgerEntry + * when iterator is called, you are handing out the entries, you may not know when the caller will Review comment: > Anyway, let's move forward without retainedIterator. It is not a blocker for 4.6.0. We can circle back in future if it is needed. Let's leave it out for now then. I think with APIs it's better to leave it out if in doubt. It's always easier to add calls to an API than remove them. Regarding retain vs retained vs retaining, in the case of ByteBuf, it is the bytebuf itself that has the reference count, so it makes sense to "retain" a bytebuf and have a "retained" bytebuf. However, in the case of an iterator, it's not the iterator that is being retained. Rather the iterator is just a container for objects that are retained, so in some sense the iterator is retaining the objects, though that's not perfect either. Another thing I thought about with the retainedIterator is that error handling becomes awkward. If while iterating an error occurs, you still need to iterate to the end to free all refcounts, which isn't immediately obvious from the api. In summary, there seems to be a good few sharp corners on this call, so best to leave it out for now. ---------------------------------------------------------------- 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