-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[feat][cli] Add command line option for configuring the memory limit #19434
Conversation
@shy-share Please add the following content to your PR description and select a checkbox:
|
pulsar-testclient/src/main/java/org/apache/pulsar/testclient/PerfClientUtils.java
Outdated
Show resolved
Hide resolved
@Parameter(names = { "-m", "--memory", }, description = "Configure the Pulsar client memory limit") | ||
Long memory=0L; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's pretty unconvenient to specify megabytes as bytes on the command line. Do we already have a parser that can accept typical units like M = megabyte etc. .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it should be changed to this ?
@parameter(names = { "-M", "--Memory", },
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shy-share IIUC it means you can specify -m 41M
means configuring memory as 41MB.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tisonkun I know what you mean but I don't know how to write this. like this
protected static final double MB = 1024 * 1024;
@Parameter(names = { "-m", "--memory", }, description = "Configure the Pulsar client memory limit{MB}")
Long memory=0*MB;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lhotari @nicoloboschi Does our CLI tools already provide such functionality? I can read broker.conf
options like:
# For Amazon S3 ledger offload, Max block size in bytes. (64MB by default, 5MB minimum)
s3ManagedLedgerOffloadMaxBlockSizeInBytes=67108864
# For Amazon S3 ledger offload, Read buffer size in bytes (1MB by default)
s3ManagedLedgerOffloadReadBufferSizeInBytes=1048576
The pr had no activity for 30 days, mark with Stale label. |
I notice that we do have some utils to implement this future. See #20646 (review). Closing as stale... But anyone can take over this work as it's clear to follow now. |
Fixes #15912
Master Issue: #xyz
PIP: #xyz
Motivation
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: