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] PIP-192 PIP-220 Added TransferShedder #18865

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

heesung-sn
Copy link
Contributor

@heesung-sn heesung-sn commented Dec 10, 2022

Master Issue: #18215

Motivation

This PR implement PIP-220, #18215

Modifications

For the PIP-220(a sub-pip of pip-192, #16691), this PR implements the TransferShedder and its unit test.

Also, there are minor changes in the related classes:

  • Added the recentlyUnloadedBrokers input var in NamespaceUnloadStrategy.findBundlesForUnloading(...)
  • Added the TransferShedder-related configs in ServiceConfiguration

Verifying this change

  • Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

  • *Added unit tests for the new classes.

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
  • Anything that affects deployment

Documentation

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

We will have separate PRs to update the Doc later.

Matching PR in forked repository

PR in forked repository: heesung-sn#16

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Dec 10, 2022
@Demogorgon314 Demogorgon314 added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker doc-required Your PR changes impact docs and you will update later. and removed doc-not-needed Your PR changes do not impact docs labels Dec 13, 2022
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-required Your PR changes impact docs and you will update later. labels Dec 13, 2022
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. Only have one comment, the configuration name is so long, can we remove the prefix?

@heesung-sn heesung-sn force-pushed the pip-220 branch 2 times, most recently from 44eb837 to 7bfb998 Compare December 16, 2022 22:14
@heesung-sn
Copy link
Contributor Author

Please hold on reviewing this PR. I will refactor this PR according to the changes in #19154.

@heesung-sn
Copy link
Contributor Author

Please continue the review. I applied the changes from #19154.

@Technoboy- Technoboy- added this to the 3.0.0 milestone Feb 2, 2023
+ "such as load balance states and decisions. "
+ "(only used in load balancer extension logics)"
)
private boolean loadBalancerDebugModeEnabled = false;
Copy link
Contributor

@gaoran10 gaoran10 Feb 2, 2023

Choose a reason for hiding this comment

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

Does this config is necessary? Maybe we can do the same thing via modifying the broker log config file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some systems do support fine-grained debugging logs for certain features.

I am curious if the pulsar follows this practice, but I think this could be useful to see debug logs from load balancer logic only.

Copy link
Member

Choose a reason for hiding this comment

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

Should we mark this configuration as dynamic? Then we can change this configuration in real time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can mark all these configs as dynamic. Updating.

Copy link
Contributor

@gaoran10 gaoran10 Feb 2, 2023

Choose a reason for hiding this comment

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

The Pulsar broker uses the log4j2, which supports custom log configurations by modifying the config file ${PULSAR_HOME}/conf/log4j2.yaml, which supports log control at the class level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's another way of fine-configuring the log level, and I assume we can control the log level by the package as well.

But with this dynamic config, we can more easily turn on/off the debugging logs too.

Copy link
Contributor

@gaoran10 gaoran10 Feb 2, 2023

Choose a reason for hiding this comment

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

OK, but it's a little strange to add a separate param to control debug log information.

@merlimat merlimat merged commit 4caf41a into apache:master Feb 2, 2023
@heesung-sn heesung-sn changed the title [improve][broker] PIP-220 Added TransferShedder [improve][broker] PIP-192 PIP-220 Added TransferShedder Feb 12, 2023
@heesung-sn heesung-sn deleted the pip-220 branch April 2, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker 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
Development

Successfully merging this pull request may close these issues.

5 participants