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] remove usage of deprecated loadBalancerMemoryResourceWeight. #22801

Closed

Conversation

thetumbled
Copy link
Member

@thetumbled thetumbled commented May 30, 2024

Motivation

loadBalancerMemoryResourceWeight has been deprecated in #19559.
But it still work at some logic.

Modifications

Remove it completely.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

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

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

Matching PR in forked repository

PR in forked repository: thetumbled#55

@thetumbled
Copy link
Member Author

PTAL, thanks. @lhotari @dao-jun @Demogorgon314 @heesung-sn

@lhotari
Copy link
Member

lhotari commented May 30, 2024

@thetumbled there's #21973 describing a way how to fix the issue in estimating the heap memory usage. if that is done, it would be possible to keep on using it. It's a major gap if the heap memory usage isn't considered at all.

@thetumbled
Copy link
Member Author

thetumbled commented May 30, 2024

@thetumbled there's #21973 describing a way how to fix the issue in estimating the heap memory usage. if that is done, it would be possible to keep on using it. It's a major gap if the heap memory usage isn't considered at all.

Do we have a plan for using this configuration again? If #21973 enhance the collection of memory usage, will memory usage have a strong relation with the actual load of the broker?
WDYT, @heesung-sn @Demogorgon314 @codelipenghui @Technoboy-

@thetumbled
Copy link
Member Author

thetumbled commented May 31, 2024

I start a discussion for this pr: https://lists.apache.org/thread/qpdvyytk8y7x8yzh35jshksnl6498y3r, looking forward to your feedback.
And this is the implementation PR for this thread: #22820

@heesung-sn
Copy link
Contributor

Removing the memory usage signals in the code is probably a radical decision here. We could disable them by default, but we could still let users use them if they want to. #21973 seems promising to improve the memory signals in the future.

@thetumbled
Copy link
Member Author

Removing the memory usage signals in the code is probably a radical decision here. We could disable them by default, but we could still let users use them if they want to. #21973 seems promising to improve the memory signals in the future.

Yes, PR #22820 is to disable them by default. PTAL, thanks.

@thetumbled
Copy link
Member Author

thetumbled commented Jun 5, 2024

As #22820 has been merged, i close this pr and looking forward to the improvement of memory usage collection.
If needed, we can still reopen this pr to remove the logic about memory usage thoroughly.

@thetumbled thetumbled closed this Jun 5, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants