Skip to content
New issue

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

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

Already on GitHub? # to your account

[fix][broker] Immediately tombstone Deleted and Free state bundles #22743

Merged
merged 4 commits into from
May 21, 2024

Conversation

heesung-sn
Copy link
Contributor

@heesung-sn heesung-sn commented May 18, 2024

Motivation

  • Extensible Load Balancer cannot split bundles when a namespace is recreated. The old parent bundle before split (in the Deleted state) could block the next split operation until they are tombstoned by the leader monitor bundle cleanup job.

  • Extensible Load Balancer delays bundle cleanups when a namespace is deleted. Those deleted bundles are stuck in Free state until they are tombstoned by the leader monitor bundle cleanup job. Although this delayed tombstone was introduced for non-transfer unloading, these lingering bundles are not ideal when the namespace is deleted.

Modifications

  • Immediately tombstone Deleted state bundles
  • Set ServiceUnitStateData.force=true when deleting(unloading) namespace bundles. Then, immediately tombstone Free state bundles if force is true.

Verifying this change

  • Make sure that the change passes the CI checks.

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: heesung-sn#67

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 18, 2024
@heesung-sn heesung-sn requested a review from Demogorgon314 May 18, 2024 13:35
@heesung-sn heesung-sn self-assigned this May 18, 2024
@heesung-sn heesung-sn changed the title [fix][broker] Immediately tombstone Deleted and Free state bundles and enable MultiPhaseBundleUnload for non-transfer unloading [fix][broker] Immediately tombstone Deleted and Free state bundles May 19, 2024
Copy link
Member

@Demogorgon314 Demogorgon314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.20%. Comparing base (bbc6224) to head (4dcd4ac).
Report is 293 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #22743      +/-   ##
============================================
- Coverage     73.57%   73.20%   -0.38%     
+ Complexity    32624    32577      -47     
============================================
  Files          1877     1889      +12     
  Lines        139502   141433    +1931     
  Branches      15299    15520     +221     
============================================
+ Hits         102638   103531     +893     
- Misses        28908    29905     +997     
- Partials       7956     7997      +41     
Flag Coverage Δ
inttests 27.48% <15.78%> (+2.90%) ⬆️
systests 24.68% <0.00%> (+0.35%) ⬆️
unittests 72.21% <100.00%> (-0.64%) ⬇️

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

Files Coverage Δ
...dbalance/extensions/ExtensibleLoadManagerImpl.java 81.34% <100.00%> (+1.25%) ⬆️
...xtensions/channel/ServiceUnitStateChannelImpl.java 85.33% <100.00%> (+0.02%) ⬆️
...r/loadbalance/extensions/manager/SplitManager.java 91.48% <100.00%> (+2.12%) ⬆️
.../loadbalance/extensions/manager/UnloadManager.java 86.79% <100.00%> (-0.97%) ⬇️
...ache/pulsar/broker/namespace/NamespaceService.java 73.58% <100.00%> (+1.35%) ⬆️

... and 350 files with indirect coverage changes

@heesung-sn heesung-sn merged commit 949260f into apache:master May 21, 2024
71 of 74 checks passed
BewareMyPower added a commit to BewareMyPower/pulsar that referenced this pull request Jun 6, 2024
…t channel

### Motivation

NPE will happen in `UnloadManager#handleEvent` after
apache#22743. It's because the `Init`
state is always associated with a null `ServiceUnitStateData`.

```
java.lang.NullPointerException: Cannot invoke "org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateData.force()" because "data" is null
        at org.apache.pulsar.broker.loadbalance.extensions.manager.UnloadManager.handleEvent(UnloadManager.java:204) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.StateChangeListeners.lambda$notify$3(StateChangeListeners.java:74) ~[classes/:?]
        at java.base/java.util.concurrent.CopyOnWriteArrayList.forEach(CopyOnWriteArrayList.java:807) ~[?:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.StateChangeListeners.notify(StateChangeListeners.java:72) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl.handleInitEvent(ServiceUnitStateChannelImpl.java:902) ~[classes/:?]
        at org.apache.pulsar.broker.loadbalance.extensions.channel.ServiceUnitStateChannelImpl.handle(ServiceUnitStateChannelImpl.java:715) ~[classes/:?]
```

### Modifications

In `UnloadManager#handleEvent`, assume `data` is null and call
`complete` directly. Fix `UnloadManagerTest`, which passes a non-null
`ServiceUnitStateData` and `Init` to `handleEvent`.
@BewareMyPower BewareMyPower added this to the 3.4.0 milestone Jun 6, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 26, 2024
heesung-sn added a commit to heesung-sn/pulsar that referenced this pull request Jun 27, 2024
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request Jun 27, 2024
…pache#22743)

(cherry picked from commit 949260f)
(cherry picked from commit d982d3b)
(cherry picked from commit 44f2e98)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Jul 1, 2024
…pache#22743)

(cherry picked from commit 949260f)
(cherry picked from commit d982d3b)
(cherry picked from commit 44f2e98)
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants