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][fn] Provide a way to set Pulsar client memory limit in Pulsar Function #23959

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

walkinggo
Copy link
Contributor

Main Issue: #23723

Motivation

see #23723 . The main work of this PR is to add a method for setting the memory limit of the Pulsar client in a Pulsar Function.

Modifications

1.Memory Limit Support:
Added MemoryLimit class to define memory constraints for Pulsar clients, which includes both an absolute value and a percentage of the maximum direct memory.
This memory configuration is applied across several components including FunctionConfig, WorkerConfig, InstanceConfig, and ThreadRuntimeFactory.
2.Class Updates:
Introduced new fields in configuration classes such as FunctionConfig, WorkerConfig, and InstanceConfig to hold memory limit settings.
Updated ThreadRuntimeFactory to handle the memory limit during initialization, ensuring that the client memory limit is respected.
3.Proto File Changes:
Updated Function.proto to include a new MemoryLimit field, allowing the configuration of memory limits directly via function details.

Documentation

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

Matching PR in forked repository

PR in forked repository: walkinggo#8

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 11, 2025
@lhotari
Copy link
Member

lhotari commented Feb 12, 2025

@walkinggo Please first make sure that the CI build passes in your own fork. I see that walkinggo#8 referenced in the PR description isn't passing.

@walkinggo
Copy link
Contributor Author

@walkinggo Please first make sure that the CI build passes in your own fork. I see that walkinggo#8 referenced in the PR description isn't passing.

The error in my PR is related to json-smart, but my code doesn't involve modifications to this component. I suspect there might be an issue with Maven Central, and I also noticed that this PR is updating the version of json-smart.

@lhotari
Copy link
Member

lhotari commented Feb 12, 2025

@walkinggo Please first make sure that the CI build passes in your own fork. I see that walkinggo#8 referenced in the PR description isn't passing.

The error in my PR is related to json-smart, but my code doesn't involve modifications to this component. I suspect there might be an issue with Maven Central, and I also noticed that this PR is updating the version of json-smart.

@walkinggo PR #23966 has been merged. Please merge origin/master to your PR to get the changes included.

@walkinggo
Copy link
Contributor Author

@walkinggo Please first make sure that the CI build passes in your own fork. I see that walkinggo#8 referenced in the PR description isn't passing.

The error in my PR is related to json-smart, but my code doesn't involve modifications to this component. I suspect there might be an issue with Maven Central, and I also noticed that this PR is updating the version of json-smart.

@walkinggo PR #23966 has been merged. Please merge origin/master to your PR to get the changes included.

The PR has been successfully merged and passed the CI checks.

@walkinggo walkinggo requested a review from lhotari February 13, 2025 06:13
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants