Use scheduleWithFixedDelay instead of scheduleAtFixedRate for java producer batch timer

[klevy-toast]

Fixes #11100

Motivation

We believe that the use of scheduleAtFixedRate in the java producer's batch timer can result in unnecessarily high thread usage, which can become especially problematic for applications that start many producers.

Modifications

Replaced the use of scheduleAtFixedRate with scheduleWithFixedDelay, which is the same behavior as previously in 2.6.x. The producer's parameter batchingMaxPublishDelay implies the use of the "delay" method instead of "rate" method as well.

Verifying this change

• Make sure that the change passes the CI checks.

This change is already covered by existing tests, such as existing pulsar client producer tests.

Testing of the performance regression can be demonstrated by using this artifact and comparing a recent release of pulsar client with a manually built SNAPSHOT version with this change:

Version 2.7.1 CPU & thread behavior

• While sending messages

- While running idle producers

- 30 second profile while sending messages

Version 2.10.0-SNAPSHOT CPU & thread behavior

• While sending messages

- While running idle producers

- 30 second profile while sending messages

These samples show fewer threads running with this change compared to 2.7.1, less time spend in batchMessageAndSend, and overall lower CPU usage -- note that this testing was done on a laptop machine, and we have observed, along with other users that this CPU regression can be much worse with more producers, smaller batch intervals, and on deployed cloud applications.

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

If yes was chosen, please highlight the changes

• Dependencies (does it add or upgrade a dependency): (no)
• The public API: (no)
• The schema: (no)
• The default values of configurations: (no)
• The wire protocol: (no)
• The rest endpoints: (no)
• The admin cli options: (no)
• Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

• no-need-doc

The general behavior of the batch timer feature should not be changing

[eolivelli]

Thanks for spending time in debugging this.

I second this patch

+1
@rdhabalia @merlimat @michaeljmarshall @lhotari @codelipenghui you may be interested in this patch

[merlimat]

Looks good.

I think we could even go further and schedule the task only when the producer is active.

[michaeljmarshall]

LGTM. This is a great catch.

For example, considering that the default delay is 1 millisecond, a 10 millisecond GC pause on the client could lead to 10 immediate calls to batchMessageAndSend(). Since the runnable acquires a lock on the whole producer, those 10 calls are likely impacting the user application.

[michaeljmarshall]

I think we could even go further and schedule the task only when the producer is active.

@merlimat - this is a great point. Essentially, we only need a scheduled to a "flush" if there are messages in the batchMessageContainer. We could even delay the flush each time that we send messages, since a client that is pushing many messages through the producer will likely hit the batch limit before the flush. Without pushing out the flush, we'll also increase the probability of under-filled batches.

@klevy-toast - are you interested in working on this fix? If not, I think we should merge this and then someone can work on a follow up fix?

I'd argue we could cherry pick this commit to older release branches since it is very well understood.

[michaeljmarshall]

After thinking about this a bit more, I think it makes sense to merge this as is, cherry-pick it to older release branches, and then any further optimizations will just target master.

[klevy-toast]

I think we could even go further and schedule the task only when the producer is active.

@merlimat - this is a great point. Essentially, we only need a scheduled to a "flush" if there are messages in the batchMessageContainer. We could even delay the flush each time that we send messages, since a client that is pushing many messages through the producer will likely hit the batch limit before the flush. Without pushing out the flush, we'll also increase the probability of under-filled batches.

@klevy-toast - are you interested in working on this fix? If not, I think we should merge this and then someone can work on a follow up fix?

I probably will not be able to work on that enhancement, but I agree that it is a great idea! This issue originally surfaced for us when we had an application running many inactive producers, but we ended up working around it by just disabling batch messaging.