[improve] [admin] Make the default value of param --get-subscription-backlog-size of admin API topics stats true

[poorbarcode]

Motivation

In the PR #9302, the property backlog size of each subscription returned in the response of the API topics stats, by default this property is always equal to 0 in response, and this will confuse users. Since the calculation of backlog size is done in broker memory, there is no significant overhead(the process is described in the following section), so I think the correct values should be displayed by default.

Discuss in mail list

• https://lists.apache.org/thread/wvjrp3co9fyl596onwv037944kdz000d

The following two APIs should be affected:

In Pulsar admin API

pulsar-admin topics stats persistent://test-tenant/ns1/tp1
--get-subscription-backlog-size
pulsar-admin topics stats persistent://test-tenant/ns1/tp1 -sbs

the default value of parameter --get-subscription-backlog-size will be true

In Pulsar Rest API

curl GET "http://127.0.0.1:8080/test-tenant/ns1/tp1/stats?subscriptionBacklogSize=true"

the default value of the parameter subscriptionBacklogSize will be true

The following is the process of calculating backlog size:

• Divide PersistentTopc.ledgers into two parts according to the ledgerId of the mark delete position of the cursor. The second part is ledgers indicating the messages still need to be consumed, aka backlogSizeInLedgers.
• Find the LedgerInfo whose ledgerId is the same as the ledgerId of the mark delete position of the cursor, and we can also divide the ledger into two parts, the second part is entries indicating the messages still need to be consumed, multiply the average size of each entry in metrics by the number of still need to be consumed entries we can get the backlog size in this ledger. aka backlogSizeInEntries.
• backlogSizeInLedgers + backlogSizeInEntries

Modifications

• Make the default value of param "--get-subscription-backlog-size" of admin API "topics stats" true
• Make the attribute 'backlogSize' in the response to be -1 when -sbs is false, for distinguish whether --get-subscription-backlog-size is set to true

Documentation

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

Matching PR in forked repository

PR in forked repository:

• [ci] [improve] [admin] Make the default value of param --get-subscription-… poorbarcode/pulsar#54

[gaoran10]

LGTM, the calculation uses the data in memory, no IO operations, and maybe we can return -1 when the option param --get-subscription-backlog-size is false.

[poorbarcode]

@gaoran10

the calculation uses the data in memory, no IO operations, and maybe we can return -1 when the option param --get-subscription-backlog-size is false.

Good idea. Already make the attribute 'backlogSize' in the response to be -1 when if -sbs is false. Please take a look again

[codecov-commenter]

Codecov Report

Merging #19302 (7f572b3) into master (457a0d5) will increase coverage by 1.40%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master   #19302      +/-   ##
============================================
+ Coverage     63.54%   64.94%   +1.40%     
- Complexity     3615    26379   +22764     
============================================
  Files          1895     1818      -77     
  Lines        137475   133776    -3699     
  Branches      15091    14692     -399     
============================================
- Hits          87355    86885     -470     
+ Misses        42240    39107    -3133     
+ Partials       7880     7784      -96     

Flag	Coverage Δ
inttests	24.91% <100.00%> (+0.73%)	⬆️
systests	25.68% <100.00%> (-0.18%)	⬇️
unittests	62.28% <100.00%> (+0.70%)	⬆️

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

Impacted Files	Coverage Δ
...pache/pulsar/broker/admin/v2/PersistentTopics.java	75.44% <ø> (-0.08%)	⬇️
...in/java/org/apache/pulsar/admin/cli/CmdTopics.java	82.33% <ø> (+1.96%)	⬆️
...mon/policies/data/stats/SubscriptionStatsImpl.java	74.74% <ø> (ø)
...ker/service/persistent/PersistentSubscription.java	69.93% <100.00%> (+1.33%)	⬆️
...g/apache/pulsar/client/api/ServiceUrlProvider.java	0.00% <0.00%> (-100.00%)	⬇️
.../apache/pulsar/websocket/data/ConsumerCommand.java	0.00% <0.00%> (-100.00%)	⬇️
...lsar/websocket/admin/v1/WebSocketProxyStatsV1.java	0.00% <0.00%> (-100.00%)	⬇️
...apache/pulsar/broker/web/MaxRequestSizeFilter.java	0.00% <0.00%> (-82.36%)	⬇️
...pache/pulsar/broker/tools/GenerateDocsCommand.java	0.00% <0.00%> (-77.28%)	⬇️
...ava/org/apache/pulsar/broker/tools/BrokerTool.java	0.00% <0.00%> (-71.43%)	⬇️
... and 262 more

[eolivelli]

@poorbarcode this is a breaking change.
We should not commit it to released branches.

[eolivelli]

cc @codelipenghui

[michaeljmarshall]

I agree, this is a change in a default, we should not cherry pick it.

[poorbarcode]

@eolivelli @michaeljmarshall

Thanks for checking my mistakes