I have submitted a fix in https://github.com/apache/pulsar/pull/17273 .
The main motivation of this PR is to fix and restore broker cache eviction of entries based on the earliest read position of active cursors (consumers) and also handle this in an efficient and performant way so that the original intention of PR #12045 [1] will be covered, the performance issue reported as #9958 [2]. Please review. -Lari [1] - https://github.com/apache/pulsar/pull/12045 [2] - https://github.com/apache/pulsar/issues/9958 On Mon, Aug 22, 2022 at 9:24 AM Michael Marshall <mmarsh...@apache.org> wrote: > Thank you for starting this important discussion, Lari. > > It seems the issue is in the way the `heap` list in the > ManagedCursorContainer class is ordered. The heap is ordered by mark > delete position, while cache eviction is determined by the slowest > read position [0]. The PR [1] essentially says that the cursor with > the oldest mark delete position is also the one with the slowest read > position. That assumption is not always true. We need to iterate over > active cursors to find the slowest read position. > > We likely want to revert [1]. It enabled caching for non-durable > cursors, which is probably helpful, but it also started tracking non > durable cursors within the ManagedCursorContainer, which is > unnecessary. > > > My assumption is also that there wouldn't have been a need to add > > https://github.com/apache/pulsar/pull/14985 at all if the broker cache > > would have worked as expected. > > With read position based cache eviction, a consumer requesting > redelivery of an entry likely triggers a read to bookkeeper because > the entry is eligible to be evicted when it is first sent to the > consumer. With this PR [2] enabled, entries are cached until passed by > the mark delete position, which could be helpful for certain workloads > that expect negative acks and want the benefit of caching. Given that > there are tradeoffs to this extra cache retention, I think it could > make sense to keep the configuration option. However, I do agree that > this feature would not work efficiently with high numbers of > individually acknowledged messages. > > Thanks, > Michael > > [0] > https://github.com/apache/pulsar/blob/81593682aa94c6ac1e526216a0ec134d1d5c5481/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java#L109 > [1] https://github.com/apache/pulsar/pull/12045 > [2] https://github.com/apache/pulsar/pull/14985 > > > On Sun, Aug 21, 2022 at 5:47 PM Lari Hotari <lhot...@apache.org> wrote: > > > > Hi all, > > > > I'd like to get some more eyes on this long outstanding performance issue > > with large fan-outs (a large number of consumers for a single topic). The > > broker cache does not work as expected due to invalid changes introduced > in > > version 2.8.2 by PR https://github.com/apache/pulsar/pull/12045. > > > > I have reported the issue with broker cache over 2 months ago with > > https://github.com/apache/pulsar/issues/16054 . Please take a look. > > > > The workaround seems to be to use the feature introduced in PR > > https://github.com/apache/pulsar/pull/14985, "Add a cache eviction > > policy：Evicting cache data by the slowest markDeletedPosition", > > by setting cacheEvictionByMarkDeletedPosition=true . > > One notable detail is that the cacheEvictionByMarkDeletedPosition entry > is > > not included in conf/broker.conf or conf/standalone.conf files. Therefore > > in the Pulsar Helm chart, it is necessary to use > > "PULSAR_PREFIX_cacheEvictionByMarkDeletedPosition: true" to activate the > > setting. > > > > However, the problem with the cacheEvictionByMarkDeletedPosition solution > > is that the cache could be filled with irrelevant entries when there's a > > lot of topics and consumers on a single broker since the cache expiration > > is based on markDeletedPosition and not the farthest behind read position > > for active consumers. > > My assumption is also that there wouldn't have been a need to add > > https://github.com/apache/pulsar/pull/14985 at all if the broker cache > > would have worked as expected. > > > > PIP-174: Provide new implementation for broker dispatch cache ( > > https://github.com/apache/pulsar/issues/15954 / > > https://github.com/apache/pulsar/pull/15955) will provide a new broker > > cache implementation where this issue hopefully goes away, but we will > > regardless need to make the fix for current maintenance versions. > > > > I'm looking forward to feedback of original contributors of the broker > > cache and the related changes so that we finally get this issue resolved. > > > > Thanks, > > > > Lari >
