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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 6 commits into from
Feb 14, 2023
Merged

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

merged 6 commits into from
Feb 14, 2023

Conversation

Demogorgon314
Copy link
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-sn heesung-sn mentioned this pull request Feb 9, 2023
Closed
heesung-sn
heesung-sn reviewed Feb 10, 2023
View reviewed changes
...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated Show resolved Hide resolved
...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated Show resolved Hide resolved
...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated
if (debugMode) {
log.info("Available brokers: {}", availableBrokers);
}
if (availableBrokers.size() <= 1) {
Copy link
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)

Copy link
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.

Copy link
Contributor

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

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.

...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated Show resolved Hide resolved
...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated Show resolved Hide resolved
...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated Show resolved Hide resolved
heesung-sn
heesung-sn reviewed Feb 10, 2023
View reviewed changes
...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated
if (unloadDecision.getUnloads().isEmpty()) {
return CompletableFuture.completedFuture(null);
}
List<CompletableFuture<Void>> futures = new ArrayList<>();
Copy link
Contributor

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

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.

Copy link
Contributor

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

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

Copy link
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.

Copy link
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?

Copy link
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
Copy link
Contributor

@gaoran10 gaoran10 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Left some trivial comments.

...java/org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadScheduler.java Outdated Show resolved Hide resolved
.../org/apache/pulsar/broker/loadbalance/extensions/scheduler/NamespaceUnloadSchedulerTest.java Outdated Show resolved Hide 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
Copy link

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
Copy link
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-sn heesung-sn heesung-sn approved these changes

@gaoran10 gaoran10 gaoran10 approved these changes

@eolivelli eolivelli eolivelli approved these changes

@Technoboy- Technoboy- Technoboy- 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-sn