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

[fix][broker] log update in class:LeastResourceUsageWithWeight #20168

Closed
wants to merge 1 commit into from

Conversation

StevenLuMT
Copy link
Member

@StevenLuMT StevenLuMT commented Apr 23, 2023

Motivation

base PR: #18964 ,log update in class(org.apache.pulsar.broker.loadbalance.extensions.strategy.LeastResourceUsageWithWeight)

Modifications

when log's level is debug,call log.debug to print log

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

Check the box below or label this PR directly.

Need to update docs?

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

Matching PR in forked repository

PR in forked repository: StevenLuMT#9

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 23, 2023
@StevenLuMT
Copy link
Member Author

/pulsarbot run-failure-checks

Copy link
Contributor

@Technoboy- Technoboy- left a comment

Choose a reason for hiding this comment

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

New loadbalancer has always been print info log with this flag.

@StevenLuMT
Copy link
Member Author

@heesung-sn help me review it ,thanks

@heesung-sn
Copy link
Contributor

We print info logs under the debugMode flag. Whats the intention?

@StevenLuMT
Copy link
Member Author

We print info logs under the debugMode flag. Whats the intention?

image

@heesung-sn In your PR #18964 ,you print info log in debugMode flag,
but I think when debugMode is true,we should print debug log in PR #20168

@heesung-sn
Copy link
Contributor

heesung-sn commented Apr 29, 2023

The intention about the info logs under this debugMode flag is to dynamically print the useful operational logs when there are issues and monitor the decision logics, without restarting brokers. One can turn off this debug mode, when done. With this dynamic flag, one may not need to change the log level as it currently outputs logs at the info level. I understand some other projects strictly matches log levels in config and code. However, it appears that Pulsar does not have such strict practice as we have other code that prints info level logs under some debug flags(e.g log.isDebugEnabled).

I would let Pulsar PMC members decide on such practice, and we follow it.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label May 31, 2023
@tisonkun
Copy link
Member

Closed as no consensus. There is no relation between debugMode and debug log level.

@tisonkun tisonkun closed this Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-not-needed Your PR changes do not impact docs ready-to-test Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants