Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[improve][broker] Improve NamespaceUnloadStrategy error message #21880

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

dragosvictor
Copy link
Contributor

@dragosvictor dragosvictor commented Jan 10, 2024

Motivation

The log message printed when failing to create the NamespaceUnloadStrategy class instance mistakenly references a different config option.
Additionally fixes setup for tests in ProxyWithExtensibleLoadManagerTest, where the option was defaulting to value org.apache.pulsar.broker.loadbalance.impl.ThresholdShedder, which is incompatible with the extensible load manager, and was producing the following error message during test runs:

2024-01-10T14:57:28,347 - ERROR - [main:UnloadScheduler@222] - Error when trying to create namespace unload strategy: org.apache.pul
sar.broker.loadbalance.impl.LeastLongTermMessageRate
java.lang.RuntimeException: org.apache.pulsar.broker.loadbalance.impl.ThresholdShedder does not implement org.apache.pulsar.broker.l
oadbalance.extensions.scheduler.NamespaceUnloadStrategy
        at org.apache.pulsar.common.util.Reflections.createInstance(Reflections.java:75) ~[pulsar-common-3.3.0-SNAPSHOT.jar:3.3.0-SN
APSHOT]
        at org.apache.pulsar.broker.loadbalance.extensions.scheduler.UnloadScheduler.createNamespaceUnloadStrategy(UnloadScheduler.j
ava:217) ~[pulsar-broker-3.3.0-SNAPSHOT.jar:3.3.0-SNAPSHOT]

Modifications

Alter the log message in org.apache.pulsar.broker.loadbalance.extensions.scheduler.UnloadScheduler#createNamespaceUnloadStrategy to properly reference config option loadBalancerLoadSheddingStrategy.
Set loadBalancerLoadSheddingStrategy to org.apache.pulsar.broker.loadbalance.extensions.scheduler.TransferShedder in ProxyWithExtensibleLoadManagerTest#configureExtensibleLoadManager.

Verifying this change

  • Make sure that the change passes the CI checks.

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: https://github.com/dragosvictor/pulsar/pull/8/files

Copy link

@dragosvictor 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 -->

@dragosvictor dragosvictor marked this pull request as ready for review January 10, 2024 22:23
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Jan 10, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (46ec730) 73.54% compared to head (69f619e) 73.55%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #21880      +/-   ##
============================================
+ Coverage     73.54%   73.55%   +0.01%     
- Complexity    32311    32327      +16     
============================================
  Files          1859     1859              
  Lines        138281   138280       -1     
  Branches      15156    15156              
============================================
+ Hits         101693   101716      +23     
+ Misses        28689    28677      -12     
+ Partials       7899     7887      -12     
Flag Coverage Δ
inttests 24.14% <0.00%> (-0.02%) ⬇️
systests 23.67% <0.00%> (-0.07%) ⬇️
unittests 72.84% <100.00%> (+0.02%) ⬆️

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

Files Coverage Δ
...dbalance/extensions/scheduler/UnloadScheduler.java 85.43% <100.00%> (-0.15%) ⬇️

... and 59 files with indirect coverage changes

@merlimat merlimat merged commit 80e3890 into apache:master Jan 11, 2024
49 of 52 checks passed
@Technoboy- Technoboy- added this to the 3.3.0 milestone Jan 11, 2024
@dragosvictor dragosvictor deleted the fix-transfer-shedder-proxy-test branch January 31, 2024 19:49
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 1, 2024
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants