Performance issue in Pulsar with large fan-outs since Pulsar 2.8.2

[Lari Hotari <lh...@apache.org>]

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

Re: Performance issue in Pulsar with large fan-outs since Pulsar 2.8.2

[Lari Hotari <lh...@apache.org>]

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 <mm...@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 <lh...@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
>

Re: Performance issue in Pulsar with large fan-outs since Pulsar 2.8.2

[Michael Marshall <mm...@apache.org>]

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 <lh...@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