[feat][cli] Add command line option for configuring the memory limit#20663
[feat][cli] Add command line option for configuring the memory limit#20663tisonkun merged 23 commits intoapache:masterfrom JooHyukKim:15912-Add-command-line-option-for-configuring-the-memory-limit
Conversation
github-actions
Bot
added
the
doc-required
|
Apologies, my mistake 🙏🏼. I was suppposed to open PR in my personal repo first. |
| @Parameter(names = { "--tlsTrustCertsFilePath" }, description = "File path to client trust certificates") | ||
| String tlsTrustCertsFilePath; | ||
|
|
||
| @Parameter(names = { "-m", "--memory", }, description = "Configure the Pulsar client memory limit") |
There was a problem hiding this comment.
Could we make this consistent with the performance commands?
Such as
@Parameter(names = { "-ml", "--memory-limit", }, description = "Configure the Pulsar client memory limit "
+ "(eg: 32M, 64M)")
There was a problem hiding this comment.
@gaoran10 Yes for sure! I was going to actually. Thank you for the reminder tho 🙏🏼
|
|
||
| @Parameter(names = { "-ml", "--memory-limit", }, description = "Configure the Pulsar client memory limit " | ||
| + "(eg: 32M, 64M)", converter = MemoryUnitToByteConverter.class) | ||
| public long memoryLimit = 0L; |
There was a problem hiding this comment.
I think this one we might be able to utilize over at PR #20646
…-line-option-for-configuring-the-memory-limit
|
We might want to proceed with #20691, so we can get rid of converters added here. Or refactor after merging might do. |
…-line-option-for-configuring-the-memory-limit
|
Let's wait til #20782 gets merged so we can make use of |
…-line-option-for-configuring-the-memory-limit
|
Blocked by #20782 |
Closing NoteTemporarily closing due to PIP-280 work is taking longer than expected. |
…-line-option-for-configuring-the-memory-limit
…ing-the-memory-limit
tisonkun
requested review from
lhotari,
michaeljmarshall,
nicoloboschi and
tisonkun
|
OWASP dependency workflow failing here, and also in the other PR. As a regular commiter, this many number of CVE's to fix is overwhelming 😭 |
|
@JooHyukKim If the OWASP failures are unrelated to this patch, it's not a blocker. Don't worry. I just too busy to give a review 🤣 |
|
/pulsarbot rerun-failure-checks |
Right (wink)(wink) 😆 thanks always! 🙏🏽🙏🏽 |
|
Converted to |
|
Let me invesitgate the license issue. |
Signed-off-by: tison <wander4096@gmail.com>
…ing-the-memory-limit
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20663 +/- ##
=============================================
+ Coverage 36.84% 73.20% +36.35%
- Complexity 12229 32500 +20271
=============================================
Files 1699 1888 +189
Lines 130559 140251 +9692
Branches 14264 15444 +1180
=============================================
+ Hits 48106 102670 +54564
+ Misses 76121 29480 -46641
- Partials 6332 8101 +1769
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| fi | ||
|
|
||
| JARS=$(tar -tf $TARBALL | grep '\.jar' | grep -v 'trino/' | grep -v '/examples/' | grep -v '/instances/' | grep -v pulsar-client | grep -v pulsar-common | grep -v pulsar-package | grep -v pulsar-websocket | grep -v bouncy-castle-bc | sed 's!.*/!!' | sort) | ||
| JARS=$(tar -tf $TARBALL | grep '\.jar' | grep -v 'trino/' | grep -v '/examples/' | grep -v '/instances/' | grep -v pulsar-client | grep -v pulsar-cli-utils | grep -v pulsar-common | grep -v pulsar-package | grep -v pulsar-websocket | grep -v bouncy-castle-bc | sed 's!.*/!!' | sort) |
There was a problem hiding this comment.
Oh... this was it. Thank you @tisonkun ! 😭
…pache#20663) Signed-off-by: tison <wander4096@gmail.com> Co-authored-by: tison <wander4096@gmail.com>
5 participants
Fixes #15912
Motivation
As described in #15912
Modifications
--memory-limitconfiguration forPerformanceBaseArgumentsMemoryUnitToByteConverterimplementing JCommander'sIStringConverter<Long>for isolating parsing logic.Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: https://github.com/JooHyukKim/pulsar/pull/14