diff --git a/pip/pip-280.md b/pip/pip-280.md new file mode 100644 index 0000000000000..bbc7fab32f5b8 --- /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 { + + private static Set 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