This is an automated email from the ASF dual-hosted git repository.
tison pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pulsar.git
The following commit(s) were added to refs/heads/master by this push:
new f69e7dd2dea [pip][design] PIP-280: Refactor CLI for measurement units
(time and byte) (#20691)
f69e7dd2dea is described below
commit f69e7dd2dea07c6ab55b367a7818218843ec9335
Author: Kim, Joo Hyuk <[email protected]>
AuthorDate: Wed Jul 12 21:06:28 2023 +0900
[pip][design] PIP-280: Refactor CLI for measurement units (time and byte)
(#20691)
Co-authored-by: tison <[email protected]>
---
pip/pip-280.md | 119 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/pip/pip-280.md b/pip/pip-280.md
new file mode 100644
index 00000000000..bbc7fab32f5
--- /dev/null
+++ b/pip/pip-280.md
@@ -0,0 +1,119 @@
+# Title: Refactor CLI Argument Parsing Logic for Measurement Units using
JCommander's custom converter
+
+## Motivation
+
+In the current Pulsar codebase, the logic to parse CLI arguments for
measurement units like time and bytes is
+scattered across various CLI classes. Each value read has its distinct parsing
implementation, leading to a lack of code
+reuse.
+
+## Goals
+
+This PIP is to refactor the argument parsing logic to leverage the
`@Parameter.converter`
+functionality provided by JCommander [link 3]. This will isolate the
measurement-specific parsing logic and increase
+code
+reusability.
+
+### In Scope
+
+- Refactor all `Cmd` classes to utilize the converter functionality of
JCommander. This will streamline the parsing
+ logic and simplify the codebase.
+- Refer to bottom section "Concrete Example", before "Links"
+- Or on-going PR with small use case in
https://github.com/apache/pulsar/pull/20663
+
+### Out of Scope
+
+- Creation of a "util" module is out of the scope of this PIP.
+
+## Design & Implementation Details
+
+- The refactoring will be carried out on a class-by-class basis or per
inner-class basis.
+- Target command classes for this refactoring include
+ - `CmdNamespaces.java`
+ - `CmdTopics.java`,
+ - `CmdTopicPolicies.java`.
+
+## Note
+
+- Additional classes may be included as the refactoring progresses.
+- Respective PRs will be added here also.
+- The refactoring should not introduce any breaking change
+- New parameters should be covered by unit test (at least by existing and
preferably new)
+
+## Concrete Example
+
+Consider the code snippet
+from
[CmdNamespaces.java](https://github.com/apache/pulsar/blob/200fb562dd4437857ccaba3850bd64b0a9a50b3c/pulsar-client-tools/src/main/java/org/apache/pulsar/admin/cli/CmdNamespaces.java#L2352-L2359)
+for example. The existing code uses a local variable `maxBlockSizeStr` to
temporarily store the value
+of `--maxBlockSize` or `-mbs`. This is then parsed and validated in a separate
section of the code.
+
+### BEFORE
+
+```java
+ @Parameter(
+ names = {"--maxBlockSize", "-mbs"},
+ description = "Max block size (eg: 32M, 64M), default is 64MB s3 and
google-cloud-storage requires this parameter",
+ required = false)
+private String maxBlockSizeStr;
+```
+
+parsing like below ....
+
+```java
+ // parsing like....
+ int
maxBlockSizeInBytes=OffloadPoliciesImpl.DEFAULT_MAX_BLOCK_SIZE_IN_BYTES;
+ if(StringUtils.isNotEmpty(maxBlockSizeStr)){
+ long maxBlockSize=validateSizeString(maxBlockSizeStr);
+ if(positiveCheck("MaxBlockSize",maxBlockSize)
+ &&maxValueCheck("MaxBlockSize",maxBlockSize,Integer.MAX_VALUE)) {
+ maxBlockSizeInBytes=Long.valueOf(maxBlockSize).intValue();
+ }
+ }
+```
+
+### AFTER
+
+```java
+ @Parameter(
+ names = {"--maxBlockSize", "-mbs"},
+ description = "Max block size (eg: 32M, 64M), default is 64MB s3 and
google-cloud-storage requires this parameter",
+ required = false, converter = MemoryUnitToByteConverter.class) // <---
parsing logic "inline" easy to follow
+private long maxBlockSizeStr=DEFAULT_MAX_BLOCK_SIZE_IN_BYTES; // <---- default
value in line
+```
+
+... and actual parsing in isolation, ready for reuse like...
+
+```java
+class MemoryUnitToByteConverter implements IStringConverter<Long> {
+
+ private static Set<Character> sizeUnit = Sets.newHashSet('k', 'K', 'm',
'M', 'g', 'G', 't', 'T');
+ private final long defaultValue;
+
+ public MemoryUnitToByteConverter(long defaultValue) {
+ this.defaultValue = defaultValue;
+ }
+
+ @Override
+ public Long convert(String memoryLimitArgument) {
+ parseBytes(memoryLimitArgument);
+ }
+
+ long parseBytes(String memoryLimitArgument) {
+ if (StringUtils.isNotEmpty(memoryLimitArgument)) {
+ long memoryLimitArg = validateSizeString(memoryLimitArgument);
+ if (positiveCheckStatic("memory-limit", memoryLimitArg)) {
+ return memoryLimitArg;
+ }
+ }
+ return defaultValue;
+ }
+ ...
+ more internal
+ helper methods
+}
+```
+
+## Links
+
+- Mailing List discussion thread:
https://lists.apache.org/thread/b77bfnjlt62w7zywcs8tqklvyokpykok
+- Mailing List voting thread:
https://lists.apache.org/thread/0r3bh0h7f86g2x9odvrd1fp2gwddq904
+- [3] https://jcommander.org/#_custom_types_converters_and_splitters
