[improve][broker] refactor ManagedLedger cacheEvictionTask implement

[aloyszhang]

Motivation

Current the cacheEvictionTask

pulsar/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java

Line 255 in 4532c15

private void cacheEvictionTask() {

will be executed periodically, the period is calculated by

  double evictionFrequency = Math.max(Math.min(config.getCacheEvictionFrequency(), 1000.0), 0.001);
  long waitTimeMillis = (long) (1000 / evictionFrequency);

And this task will be executed once and then sleep waitTimeMillis before the next round.

We can do some optimizes for this logic.

Modifications

1. Set the cache eviction task interval directly by addint a new configuration managedLedgerCacheEvictionIntervalMs
2. use a ScheduledExecutorService to execute the cache eviction task every managedLedgerCacheEvictionIntervalMs

Verifying this change

This change is already covered by existing tests, such as (please describe tests).

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

• no-need-doc

  (Please explain why)

[github-actions]

@aloyszhang:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

[github-actions]

@aloyszhang:Thanks for providing doc info!

[codelipenghui]

Hi @aloyszhang Do we have an email discussion for the new configuration? Please add the discussion link to the PR description, so that we can push forward this PR.

[aloyszhang]

@codelipenghui , Sorry for being late, I have sent the discussion email right now.

Discuss thread: https://lists.apache.org/thread/wryzorp7jt6511jt25cv7mss4lz3r1bt

[aloyszhang]

/pulsarbot run-failure-checks

[aloyszhang]

/pulsarbot run-failure-checks

[github-actions]

The pr had no activity for 30 days, mark with Stale label.

[github-actions]

The pr had no activity for 30 days, mark with Stale label.

[aloyszhang]

/pulsarbot run-failure-checks

[lhotari]

It would be good to fix #16054 before changing the cache.

[aloyszhang]

/pulsarbot run-failure-checks

[aloyszhang]

It would be good to fix #16054 before changing the cache.

@lhotari This pull request just refactors the eviction from an eviction and sleep loop to a regularly scheduled task and does not change any logic of the eviction task. IMO, this PR does not relate to #16054

[mattisonchao]

LGTM, I left some little questions. I hope you can answer.

[aloyszhang]

/pulsarbot run-failure-checks

[michaeljmarshall]

Was this discussed on the mailing list? I did a quick search and couldn't find a thread on the topic. It's especially important to give this PR visibility because it changed the names of configuration in a breaking way.

We can do some optimizes for this logic.

@aloyszhang - did you benchmark this change? I thought the previous implementation was intentionally designed to decrease GC overhead.

[lhotari]

It would be good to fix #16054 before changing the cache.

@lhotari This pull request just refactors the eviction from an eviction and sleep loop to a regularly scheduled task and does not change any logic of the eviction task. IMO, this PR does not relate to #16054

@aloyszhang I was just trying to get the most critical problem of the broker cache (#16054) fixed before we do any other changes. I have now pushed a fix in #17273 . Please review since it should make a difference for heavy users of Pulsar.