[improve] Do not process acks in the Netty thread

[dlg99]

Motivation

Cherry-picked from internal @eolivelli work.
Do not block netty threads if cursor has a lot of individuallyDeletedMessages or needs to compress the PositionInfo (separate change)

Modifications

Run acks on pulsar executor

Verifying this change

• Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

NO

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: dlg99#16

[lhotari]

I think that an ordered executor (or pinned single thread executor) should be used so that all acknowledgements for a single subscription/cursor or topic get processed in a single thread.

[dlg99]

@lhotari something like topic.getBrokerService().getTopicOrderedExecutor().chooseThread(topicName)?
I'd love to avoid adding yet another executor.

[lhotari]

@dlg99 yes makes sense. I guess that the thread selection should involve the subscription name so that acknowledgements for a single subscription get handled by a single thread so that there would be less contention.

[lhotari]

Do not block netty threads if cursor has a lot of individuallyDeletedMessages or needs to compress the PositionInfo (separate change)

@dlg99 @eolivelli Do you have any performance results to share? What is the improvement when this PR is applied? In what type of use cases does this have an impact?

[eolivelli]

I don't have numbers to share because we did this work in the scope of a case in which an application generates huge lists of individuallyDeletedMessages ranges (millions).

We have other patches to share that will allow compressing the cursor PositionInfo while writing to BookKeeper as well (and also chunk the payload if it doesn't fit 5MB).
We also have some other patches to improve the memory overhead caused by Protobuf (it generates tons of garbage on the heap)

As the serialization happens when you call "subscription.acknowledge(xx)" you can see the netty thread blocked for milliseconds.

[dlg99]

@lhotari @eolivelli any objections to merging this? (PR needs an approval)

[lhotari]

@lhotari @eolivelli any objections to merging this? (PR needs an approval)

I do have concerns. With asynchronous methods, there's usually a problem with back pressure. (another example: #22541 (comment)) Perhaps it's not a real issue for acknowledgements.
Has this solution been proven with heavy testing and/or production usage?

[merlimat]

I think the premise that the code is "blocking the IO thread" is not super accurate.
There is no blocking behavior here (eg: waiting for some other operation to complete). It might be doing some significant amount of work and consume non-trivial amount of CPU, though that would be happening anyway in the other thread.

At the same time, this change will introduce an extra context switch for everyone else, even when amount of work to do for the ack is 0.