[fix][broker] fix getMaxReadPosition in TransactionBufferDisable should return latest

[TakaHiR07]

Fixes #23027

Motivation

As shown in the issue, getMaxReadPosition in TransactionBufferDisable should return latest. This implementation is changed in #21466

Modifications

getMaxReadPosition in TransactionBufferDisable return latest

Verifying this change

• Make sure that the change passes the CI checks.

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

[lhotari]

The change makes sense to me since there's other logic in ManagedLedger/ManagedCursor to avoid reading beyond last confirmed entry.

@TakaHiR07 Would it be possible to add some tests?

[TakaHiR07]

The change makes sense to me since there's other logic in ManagedLedger/ManagedCursor to avoid reading beyond last confirmed entry.

@TakaHiR07 Would it be possible to add some tests?

It is hard to add test for the case in #23027.

Besides, This pr just revert the getMaxReadPosition() implementation to version-2.9.x, but there is a potential issue may be need to consider. DispatcherRateLimiter may not work well in a corner case.

For example, one topic set a small rate for topic-level dispatcherRateLimiter, and then multiple subscription try to consume one by one. But topic has no new message writing, so all the subscription would become waitingCursor and wait to be notify once new message publish successful. So if new message publish, all the subscription can read message in the same time, maybe exceed the rate of dispatch.

To some degree, getMaxReadPosition() return lastConfirmedEntry can avoid this corner case . But it is not reasonable because when disable transaction, the maxReadPosition should be latest. Other code implementation should obey this point or it may bring unexpected error.

In all, this potential corner issue should not block this pr

[lhotari]

Oddly, this change seems to trigger a lot of test failures which might be test flakiness, invalid tests or some other underlying bug.

[lhotari]

@TakaHiR07 A lot of tests break consistently with this change. Do you have a chance to investigate?

[TakaHiR07]

@TakaHiR07 A lot of tests break consistently with this change. Do you have a chance to investigate?

@lhotari Have fixed, The reason is also the change in pr-21466. Previous pr try to fix a issue about transaction, but there is no need to change the getMaxReadPosition() about non-transaction. So the ServerCnx#getLastMessageId() should return lastConfirmedEntry when transaction disable, as the same as the implementation in before pulsar version.

[lhotari]

LGTM

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.26%. Comparing base (fbc50b0) to head (9df0df5).

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #24898      +/-   ##
============================================
- Coverage     74.34%   74.26%   -0.09%     
- Complexity    33532    33885     +353     
============================================
  Files          1913     1913              
  Lines        149419   149422       +3     
  Branches      17356    17357       +1     
============================================
- Hits         111089   110969     -120     
- Misses        29480    29600     +120     
- Partials       8850     8853       +3     

Flag	Coverage Δ
inttests	26.32% <60.00%> (-0.39%)	⬇️
systests	22.73% <60.00%> (-0.11%)	⬇️
unittests	73.80% <100.00%> (-0.06%)	⬇️

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

Files with missing lines	Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java	79.37% <100.00%> (+0.18%)	⬆️
...nsaction/buffer/impl/TransactionBufferDisable.java	57.57% <100.00%> (ø)

... and 84 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.