[GitHub] [pulsar] michaeljmarshall commented on pull request #17105: [fix][broker] Duplicate ByteBuffer when Caching Backlogged Consumers

[GitBox <gi...@apache.org>]

michaeljmarshall commented on PR #17105:
URL: https://github.com/apache/pulsar/pull/17105#issuecomment-1216125337

   > Small suggestion:
   > 
   > Could we make the `insert` method ensure it? It can avoid the same problem when anybody inserts it in the future. Maybe we can make `cachedData = entry.getDataBuffer().retain();` use `retainedDuplicate()`. Or change the insert method to `insert(EntryImpl entry, boolean duplicateDataBuffer)` and the `duplicateDataBuffer` default is `true`. you can set it to `false` when you ensure you never change the original `ByteBuffer`. (Just quick think and still need deep thinking).
   > 
   > The advantage is that we make this method more fault-tolerant and don't need create another `EntryImpl`
   
   @mattisonchao - I didn't do it this way because the write path for tail read caching does not need the duplicate `ByteBuffer` created. That path technically doesn't even need an `Entry`, but it creates one because `insert` only takes an `Entry`. It seems like we should consider expanding/improving the `EntryCache` interface to meet these two different use cases. I agree that we should think about simplifying the entry reference management. I don't have benchmarks to show how much it costs to create an `Entry` unnecessarily, but this part of the code base is highly optimized, so I would prefer not to introduce unnecessary duplication if we can avoid it.
   
   I am going to merge this as is so that it does not hold up the 2.11 release at all. We can definitely continue this discussion, especially because we have the PIP on improved caching getting implemented right now.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org