[fix][broker] Fix cluster level OffloadedReadPriority to bookkeeper-first does not work #24151

hangc0276 · 2025-04-05T01:06:32Z

Fixes #xyz

Main Issue: #xyz

PIP: #xyz

Motivation

When we set cluster level managedLedgerDataReadPriority to bookkeeper-first in conf/broker.conf

Lines 1262 to 1264 in f2618c1

    
           # Read priority when ledgers exists in both bookkeeper and the second layer storage 
        
           # (tiered-storage-first/bookkeeper-first, default is tiered-storage-first) 
        
           managedLedgerDataReadPriority=tiered-storage-first

Then use the command bin/pulsar-admin namespaces set-offload-threshold -s 0 <namespace> to set the offload threshold in namespace level to trigger topic offload automatically.

However, the broker side will create an empty OffloadPolicy when setting the offload threshold

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java

Lines 2174 to 2176 in f2618c1

    
           if (policies.offload_policies == null) { 
        
               policies.offload_policies = new OffloadPoliciesImpl(); 
        
           }

However, the empty OffloadPolicy will set the OffloadedReadPriority to TIERED_STORAGE_FIRST in namespace level.

pulsar/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java

Lines 98 to 99 in f2618c1

    
           public static final OffloadedReadPriority DEFAULT_OFFLOADED_READ_PRIORITY = 
        
                   OffloadedReadPriority.TIERED_STORAGE_FIRST;

The namespace level offload policy has higher priority than broker level offload policy, and it makes setting broker-level OffloadedReadPriority to bookkeeper-first doesn't work

Modifications

Change OffloadPolicy's default OffloadedReadPriority to null to allow broker-level offload policy can take affect.

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.

(or)

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

(or)

This change added tests and can be verified as follows:

(example:)

Added integration tests for end-to-end deployment with large payloads (10MB)
Extended integration test for recovery after broker failure

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

If the box was checked, please highlight the changes

Documentation

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

Matching PR in forked repository

PR in forked repository:

github-actions · 2025-04-05T01:07:00Z

@hangc0276 Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

…iority_do_not_work

codecov-commenter · 2025-04-14T04:45:28Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.17%. Comparing base (bbc6224) to head (2b3de76).
⚠️ Report is 1230 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (bbc6224) and HEAD (2b3de76). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (bbc6224) HEAD (2b3de76)

unittests 2 1

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #24151       +/-   ##
=============================================
- Coverage     73.57%   39.17%   -34.40%     
+ Complexity    32624    12646    -19978     
=============================================
  Files          1877     1807       -70     
  Lines        139502   143089     +3587     
  Branches      15299    16591     +1292     
=============================================
- Hits         102638    56055    -46583     
- Misses        28908    79636    +50728     
+ Partials       7956     7398      -558

Flag	Coverage Δ
inttests	`27.63% <100.00%> (+3.04%)`	⬆️
systests	`23.80% <100.00%> (-0.53%)`	⬇️
unittests	`34.40% <100.00%> (-38.44%)`	⬇️

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

Files with missing lines	Coverage Δ
...lsar/common/policies/data/OffloadPoliciesImpl.java	`36.79% <100.00%> (-48.87%)`	⬇️

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

…irst does not work (#24151) ### Motivation When we set cluster level `managedLedgerDataReadPriority` to `bookkeeper-first` in conf/broker.conf https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/conf/broker.conf#L1262-L1264 Then use the command `bin/pulsar-admin namespaces set-offload-threshold -s 0 <namespace>` to set the offload threshold in namespace level to trigger topic offload automatically. However, the broker side will create an empty OffloadPolicy when setting the offload threshold https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L2174-L2176 However, the empty OffloadPolicy will set the `OffloadedReadPriority` to `TIERED_STORAGE_FIRST` in namespace level. https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java#L98-L99 The namespace level offload policy has higher priority than broker level offload policy, and it makes setting broker-level `OffloadedReadPriority` to `bookkeeper-first` doesn't work ### Modifications Change OffloadPolicy's default OffloadedReadPriority to `null` to allow broker-level offload policy can take affect. (cherry picked from commit 153fce9)

…irst does not work (apache#24151) ### Motivation When we set cluster level `managedLedgerDataReadPriority` to `bookkeeper-first` in conf/broker.conf https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/conf/broker.conf#L1262-L1264 Then use the command `bin/pulsar-admin namespaces set-offload-threshold -s 0 <namespace>` to set the offload threshold in namespace level to trigger topic offload automatically. However, the broker side will create an empty OffloadPolicy when setting the offload threshold https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L2174-L2176 However, the empty OffloadPolicy will set the `OffloadedReadPriority` to `TIERED_STORAGE_FIRST` in namespace level. https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java#L98-L99 The namespace level offload policy has higher priority than broker level offload policy, and it makes setting broker-level `OffloadedReadPriority` to `bookkeeper-first` doesn't work ### Modifications Change OffloadPolicy's default OffloadedReadPriority to `null` to allow broker-level offload policy can take affect. (cherry picked from commit 153fce9) (cherry picked from commit 02257c8)

…irst does not work (apache#24151) ### Motivation When we set cluster level `managedLedgerDataReadPriority` to `bookkeeper-first` in conf/broker.conf https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/conf/broker.conf#L1262-L1264 Then use the command `bin/pulsar-admin namespaces set-offload-threshold -s 0 <namespace>` to set the offload threshold in namespace level to trigger topic offload automatically. However, the broker side will create an empty OffloadPolicy when setting the offload threshold https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L2174-L2176 However, the empty OffloadPolicy will set the `OffloadedReadPriority` to `TIERED_STORAGE_FIRST` in namespace level. https://github.com/apache/pulsar/blob/f2618c15bb5a9f3fcb577068584df5d0e2e4f335/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/OffloadPoliciesImpl.java#L98-L99 The namespace level offload policy has higher priority than broker level offload policy, and it makes setting broker-level `OffloadedReadPriority` to `bookkeeper-first` doesn't work ### Modifications Change OffloadPolicy's default OffloadedReadPriority to `null` to allow broker-level offload policy can take affect.

Fix cluster level OffloadedReadPriority does not work

676397a

hangc0276 requested review from horizonzy, poorbarcode, shibd and zymap April 5, 2025 01:06

hangc0276 self-assigned this Apr 5, 2025

github-actions bot added the doc-label-missing label Apr 5, 2025

hangc0276 added type/bug The PR fixed a bug or issue reported a bug area/tieredstorage release/3.3.7 release/4.0.5 and removed doc-label-missing labels Apr 5, 2025

github-actions bot added the doc-label-missing label Apr 5, 2025

hangc0276 changed the title ~~Fix cluster level OffloadedReadPriority to bookkeeper-first does not work~~ [fix][broker] Fix cluster level OffloadedReadPriority to bookkeeper-first does not work Apr 5, 2025

hangc0276 added the ready-to-test label Apr 5, 2025

zymap approved these changes Apr 5, 2025

View reviewed changes

github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Apr 5, 2025

Merge branch 'master' into chenhang/fix_cluster_level_OffloadedReadPr…

2b3de76

…iority_do_not_work

dao-jun approved these changes Apr 9, 2025

View reviewed changes

hangc0276 closed this Apr 14, 2025

hangc0276 reopened this Apr 14, 2025

liudezhi2098 approved these changes Apr 21, 2025

View reviewed changes

hangc0276 merged commit 153fce9 into apache:master Apr 22, 2025
101 of 104 checks passed

hangc0276 added cherry-picked/branch-3.3 cherry-picked/branch-4.0 labels Apr 22, 2025

lhotari added release/3.3.8 release/3.3.7 and removed release/3.3.7 release/3.3.8 labels May 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[fix][broker] Fix cluster level OffloadedReadPriority to bookkeeper-first does not work #24151

[fix][broker] Fix cluster level OffloadedReadPriority to bookkeeper-first does not work #24151

Uh oh!

hangc0276 commented Apr 5, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Apr 5, 2025

Uh oh!

codecov-commenter commented Apr 14, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

	# Read priority when ledgers exists in both bookkeeper and the second layer storage
	# (tiered-storage-first/bookkeeper-first, default is tiered-storage-first)
	managedLedgerDataReadPriority=tiered-storage-first

	if (policies.offload_policies == null) {
	policies.offload_policies = new OffloadPoliciesImpl();
	}

	public static final OffloadedReadPriority DEFAULT_OFFLOADED_READ_PRIORITY =
	OffloadedReadPriority.TIERED_STORAGE_FIRST;

[fix][broker] Fix cluster level OffloadedReadPriority to bookkeeper-first does not work #24151

[fix][broker] Fix cluster level OffloadedReadPriority to bookkeeper-first does not work #24151

Uh oh!

Conversation

hangc0276 commented Apr 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

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

Documentation

Matching PR in forked repository

Uh oh!

github-actions bot commented Apr 5, 2025

Uh oh!

codecov-commenter commented Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hangc0276 commented Apr 5, 2025 •

edited

Loading

codecov-commenter commented Apr 14, 2025 •

edited

Loading