Visitar URL original
[improve][broker] PIP-192 Added namespace unload scheduler by Demogorgon314 · Pull Request #19477 · apache/pulsar · GitHub
Skip to content

[improve][broker] PIP-192 Added namespace unload scheduler - #19477

Merged
Demogorgon314 merged 6 commits into
apache:masterfrom
Demogorgon314:Demogorgon314/PIP-192-schedule-the-unload-job
Feb 14, 2023
Merged

[improve][broker] PIP-192 Added namespace unload scheduler#19477
Demogorgon314 merged 6 commits into
apache:masterfrom
Demogorgon314:Demogorgon314/PIP-192-schedule-the-unload-job

Conversation

@Demogorgon314

@Demogorgon314 Demogorgon314 commented Feb 9, 2023

Copy link
Copy Markdown
Member

PIP-192: #16691

Motivation

We will start raising PRs to implement PIP-192, #16691

Modifications

This PR added a namespace unload scheduler and tests.

Documentation

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

@Demogorgon314 Demogorgon314 added the type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages label Feb 9, 2023
@Demogorgon314 Demogorgon314 self-assigned this Feb 9, 2023
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Feb 9, 2023
@Demogorgon314
Demogorgon314 force-pushed the Demogorgon314/PIP-192-schedule-the-unload-job branch 2 times, most recently from 83d58f8 to 95c4e96 Compare February 9, 2023 10:06
@Demogorgon314
Demogorgon314 marked this pull request as ready for review February 9, 2023 10:07
@heesung-sohn heesung-sohn mentioned this pull request Feb 9, 2023
Closed
heesung-sohn
heesung-sohn reviewed Feb 10, 2023
View reviewed changes
Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Resolved
Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Resolved
Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java
if (debugMode) {
log.info("Available brokers: {}", availableBrokers);
}
if (availableBrokers.size() <= 1) {

@heesung-sohn heesung-sohn Feb 10, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we move this logic to TransferShedder?( and accordingly emit the decision result)

@Demogorgon314 Demogorgon314 Feb 11, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why do we need to move to TransferShedder? Are there any benefits? If only one broker is available, we don't need to do to unload.

@heesung-sohn heesung-sohn Feb 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I thought in order to isolate shedding logic, all the major shedding decisions should happen in Shedding Strategy, but ok. Im fine with this outer logic too.

Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Resolved
Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Resolved
Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Resolved
heesung-sohn
heesung-sohn reviewed Feb 10, 2023
View reviewed changes
Comment thread ...src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java Resolved
heesung-sohn
heesung-sohn reviewed Feb 10, 2023
View reviewed changes
Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java
if (unloadDecision.getUnloads().isEmpty()) {
return CompletableFuture.completedFuture(null);
}
List<CompletableFuture<Void>> futures = new ArrayList<>();

@heesung-sohn heesung-sohn Feb 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Add a member var private final Map<String, SplitDecision> inFlightUnloads to dedup the in-flight unloads. We need to remove the item when BSC.closeServiceUnit() is done.

@heesung-sohn heesung-sohn Feb 11, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here, I think we need SplitManager and UnloadManager to manage the lifecycle of the bundle split and unload requests from each broker.

  • dedup inflight bundles' unload/split event pub messages.
  • update the success counters when the unload(closeServiceUnit) and split(splitServiceUnit) is complete.

Here is my temp code where I am trying to introduce SplitManager for the same reason.
https://github.com/heesung-sn/pulsar/pull/30/files#r1103463024

@heesung-sohn heesung-sohn Feb 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or, I am fine with moving forward with the current code(without dedup) and refactoring it later when other PRs get merged.

@Demogorgon314 Demogorgon314 Feb 11, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We need to remove the item when BSC.closeServiceUnit() is done.

Good catch! Since only the channel owner can do the bundle unload, removing the item when BSC.closeServiceUnit() might not be a good idea. Maybe we should remove it when it received an Owned state?

@heesung-sohn heesung-sohn Feb 11, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, thats better. We can add this logic in handleOwned.

gaoran10
gaoran10 reviewed Feb 11, 2023
View reviewed changes

@gaoran10 gaoran10 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left some trivial comments.

Comment thread ...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Resolved
Comment thread .../org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadSchedulerTest.java Resolved
@Demogorgon314
Demogorgon314 force-pushed the Demogorgon314/PIP-192-schedule-the-unload-job branch from 95c4e96 to 1c7bdc4 Compare February 11, 2023 04:17
@Technoboy- Technoboy- added this to the 3.0.0 milestone Feb 12, 2023
@codecov-commenter

codecov-commenter commented Feb 12, 2023

Copy link
Copy Markdown

Codecov Report

Merging #19477 (5c2cc09) into master (5c8f929) will increase coverage by 0.71%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19477      +/-   ##
============================================
+ Coverage     63.30%   64.02%   +0.71%     
- Complexity    26123    26252     +129     
============================================
  Files          1836     1837       +1     
  Lines        134416   134498      +82     
  Branches      14772    14782      +10     
============================================
+ Hits          85087    86106    +1019     
+ Misses        41649    40539    -1110     
- Partials       7680     7853     +173
Flag Coverage Δ
inttests 24.82% <0.00%> (-0.06%) ⬇️
systests 25.37% <0.00%> (-0.19%) ⬇️
unittests 61.26% <0.00%> (+0.62%) ⬆️

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

Impacted Files Coverage Δ
...dbalance/extensions/ExtensibleLoadManagerImpl.java 5.75% <0.00%> (-0.13%) ⬇️
...dbalance/extensions/scheduler/UnloadScheduler.java 0.00% <0.00%> (ø)
...che/pulsar/client/api/MessagePayloadProcessor.java 12.50% <0.00%> (-87.50%) ⬇️
...apache/pulsar/broker/web/MaxRequestSizeFilter.java 0.00% <0.00%> (-82.36%) ⬇️
...lance/extensions/channel/ServiceUnitStateData.java 0.00% <0.00%> (-77.78%) ⬇️
...pache/pulsar/broker/tools/GenerateDocsCommand.java 0.00% <0.00%> (-77.28%) ⬇️
.../pulsar/client/impl/MessagePayloadContextImpl.java 0.00% <0.00%> (-74.47%) ⬇️
...ava/org/apache/pulsar/broker/tools/BrokerTool.java 0.00% <0.00%> (-71.43%) ⬇️
...ersistentStickyKeyDispatcherMultipleConsumers.java 0.00% <0.00%> (-50.00%) ⬇️
...rvice/schema/KeyValueSchemaCompatibilityCheck.java 21.62% <0.00%> (-45.95%) ⬇️
... and 185 more

@eolivelli

eolivelli commented Feb 13, 2023

Copy link
Copy Markdown
Contributor

@Demogorgon314 please rebase

@Demogorgon314
Merge branch 'master' into Demogorgon314/PIP-192-schedule-the-unload-job
9dc2e90 
# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java
@Demogorgon314
Fix test
905fa30
@Demogorgon314
Demogorgon314 merged commit af1b6e1 into apache:master Feb 14, 2023
@Demogorgon314
Demogorgon314 deleted the Demogorgon314/PIP-192-schedule-the-unload-job branch February 14, 2023 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@heesung-sohn heesung-sohn heesung-sohn approved these changes
@Technoboy- Technoboy- Technoboy- approved these changes
@eolivelli eolivelli eolivelli approved these changes
@gaoran10 gaoran10 gaoran10 approved these changes

Assignees

@Demogorgon314 Demogorgon314

Labels

doc-not-needed Your PR changes do not impact docs type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Milestone

3.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@Demogorgon314 @codecov-commenter @eolivelli @Technoboy- @gaoran10 @heesung-sohn