forked from apache/pulsar
-
Notifications
You must be signed in to change notification settings - Fork 1
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[pip][design] PIP-280: Refactor CLI for measurement units (time and b…
…yte) (apache#20691) Co-authored-by: tison <[email protected]>
1 parent
3116abf
commit f69e7dd
Showing
1 changed file
with
119 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |