[improve][broker] Add missing configuration keys for caching catch-up reads

[lhotari]

Motivation

PR #12258 added support for caching catch-up reads. However, the PR missed adding the configuration keys to broker.conf.

Modifications

• add similar configuration values as PR [pulsar-broker] Support caching to drain backlog consumers #12258 added to standalone.conf.
• make ServiceConfiguration.java defaults match the defaults in broker.conf
  • the values in PR's standalone.conf weren't the same as the defaults in ServerConfiguration. This was raised in a comment on PR #12258, but that wasn't addressed.

Documentation

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

[hangc0276]

Do we need to update the default value here?

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

Lines 86 to 87 in 0c9d860

	private int minimumBacklogEntriesForCaching = 1000;
	private int maxBacklogBetweenCursorsForCaching = 1000;

[lhotari]

Do we need to update the default value here?

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

Lines 86 to 87 in 0c9d860

	private int minimumBacklogEntriesForCaching = 1000;
	private int maxBacklogBetweenCursorsForCaching = 1000;

There's a test that makes sure that the defaults match in broker.conf and ServiceConfiguration.java . The question about the difference was never replied in the original PR by @rdhabalia, so we just have to make a guess of a good default since the original PR has 2 set of values: in standalone.conf and in ServiceConfiguration.java.

I'll revisit the changes in this PR so that I'll make the values match what is in ServiceConfiguration.java since that's what has been in use until now.

[hanmz]

LGTM

outdated review

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.63%. Comparing base (bbc6224) to head (162e2a0).
Report is 66 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #22295      +/-   ##
============================================
+ Coverage     73.57%   73.63%   +0.05%     
+ Complexity    32624    32237     -387     
============================================
  Files          1877     1887      +10     
  Lines        139502   139453      -49     
  Branches      15299    15293       -6     
============================================
+ Hits         102638   102682      +44     
+ Misses        28908    28808     -100     
- Partials       7956     7963       +7     

Flag	Coverage Δ
inttests	26.92% <ø> (+2.33%)	⬆️
systests	24.48% <ø> (+0.15%)	⬆️
unittests	72.88% <ø> (+0.04%)	⬆️

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

see 88 files with indirect coverage changes

[Technoboy-]

Do we need to update the default value here?

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

Lines 86 to 87 in 0c9d860

	private int minimumBacklogEntriesForCaching = 1000;
	private int maxBacklogBetweenCursorsForCaching = 1000;

There's a test that makes sure that the defaults match in broker.conf and ServiceConfiguration.java . The question about the difference was never replied in the original PR by @rdhabalia, so we just have to make a guess of a good default since the original PR has 2 set of values: in standalone.conf and in ServiceConfiguration.java.

I'll revisit the changes in this PR so that I'll make the values match what is in ServiceConfiguration.java since that's what has been in use until now.

Before we get the response from @rdhabalia , we'd better not merge this patch.

[lhotari]

Before we get the response from @rdhabalia , we'd better not merge this patch.

@Technoboy- Why? This PR isn't doing anything harmful. We can always revisit later.