[improve][broker] Reduce cpu usage of InMemoryDelayedDeliveryTracker.

[thetumbled]

Motivation

As https://lists.apache.org/thread/3dc1c3n7d0tmwm8974qh9w771jzgy6f6 show, method getNumberOfDelayedMessages is called frequently and the current implementation of this method is time cosuming, we need to implement this method with a much more efficient approach.

Modifications

Introduce an AtomicLong delayedMessagesCount.

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

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

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

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: thetumbled#74

[crossoverJie]

LGTM

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.28%. Comparing base (bbc6224) to head (177b3be).
Report is 1155 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #24430      +/-   ##
============================================
+ Coverage     73.57%   74.28%   +0.71%     
+ Complexity    32624    32343     -281     
============================================
  Files          1877     1868       -9     
  Lines        139502   145385    +5883     
  Branches      15299    16635    +1336     
============================================
+ Hits         102638   108006    +5368     
+ Misses        28908    28843      -65     
- Partials       7956     8536     +580     

Flag	Coverage Δ
inttests	26.75% <66.66%> (+2.17%)	⬆️
systests	23.33% <0.00%> (-1.00%)	⬇️
unittests	73.78% <100.00%> (+0.94%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...broker/delayed/InMemoryDelayedDeliveryTracker.java	91.57% <100.00%> (+3.44%)	⬆️

... and 1086 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[codelipenghui]

@thetumbled, Thanks for the quick fix.

I will remove the label release/4.0.6 since we only ship bug fixes for the LTS branch. If you still think it's required to cherry-pick the memory improvement to branch-4.0, we can discuss under the mailing list. Sometimes we need to handle something case by case.

[thetumbled]

@thetumbled, Thanks for the quick fix.

I will remove the label release/4.0.6 since we only ship bug fixes for the LTS branch. If you still think it's required to cherry-pick the memory improvement to branch-4.0, we can discuss under the mailing list. Sometimes we need to handle something case by case.

I'm fine. I'm wondering which branch is the so-called 'feature branch'.

[lhotari]

I forgot to submit my review comments that I had made earlier.

[lhotari]

On line 220 there's if (delayedMessageMap.isEmpty()) {. Would it make sense to add this.delayedMessagesCount.set(0); also to that code block?

[thetumbled]

There is no need to do that, these logic is used for feature fixedDelayDetectionLookahead.

if (delayedMessageMap.isEmpty()) {
            // Reset to initial state
            highestDeliveryTimeTracked = 0;
            messagesHaveFixedDelay = true;
        }

When the delayedMessageMap is empty, the value of delayedMessagesCount must be 0; otherwise, there are other issues that need to be resolved.

[lhotari]

When the delayedMessageMap is empty, the value of delayedMessagesCount must be 0; otherwise, there are other issues that need to be resolved.

That's true. I think it would make sense to add an error log if delayedMessageCount isn't 0 at this point. That would help detect possible issues.

[thetumbled]

That is fine. Should i create a new PR to add the debug log?

[lhotari]

@thetumbled I think it makes sense. It shouldn't be a debug log, but an error log if that ever happens. In addition, I think it should reset the count to 0, so that the state would be fixed.

[thetumbled]

Hi, Lari. I have created a pr to check this. But i think it should not be set to 0 because of the posibility of concurrent execution.
Suppose there is another thread that adds a message to the map after the check if (delayedMessageMap.isEmpty()) {; in that case, the value should not be zero, correct?
I suggest we had better keep it unchanged and only leave a warning log.

[codelipenghui]

I'm fine. I'm wondering which branch is the so-called 'feature branch'.

@thetumbled Here is the documentation for release policy https://pulsar.apache.org/contribute/release-policy/