-
Notifications
You must be signed in to change notification settings - Fork 1
Command line parser #23
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
Conversation
|
||
// Default set to false because causes loss of records from Biovotion data. https://github.com/RADAR-base/Restructure-HDFS-topic/issues/16 | ||
@Parameter(names = { "-d", "--deduplicate" }, description = "Boolean to define if to use deduplication or not.", validateWith = BooleanValidator.class) | ||
public Boolean deduplicate = false; |
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.
jcommander supports on/off flags, right? Boolean should not be needed, just the -d
flag. Like --help
} | ||
|
||
USE_GZIP = "gzip".equalsIgnoreCase(commandLineArgs.compression); | ||
DO_DEDUPLICATE = commandLineArgs.deduplicate; |
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.
If these parameters are explicitly parsed anyway, perhaps just pass them to the class instead of having them static?
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.
In that case, the restructureRecords could possibly take a builder pattern.
@Parameter(names = { "-u", "--hdfs-uri" }, description = "The HDFS uri to connect to. Eg - 'hdfs://<HOST>:<RPC_PORT>/<PATH>'.", required = true, validateWith = { HdfsUriValidator.class, PathValidator.class }) | ||
public String hdfsUri; | ||
|
||
@Parameter(names = { "-i", "--hdfs-root-directory" }, description = "The input HDFS root directory from which files are to be read. Eg - '/topicAndroidNew'", required = true, validateWith = PathValidator.class) |
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.
How about we choose between having a URI or multiple paths? Having both a path and an URI seems overkill... So
command <input_uri> <output_dir>
# or
command -u <uri> -o <dir> <input1> [<input2> ...]
@blootsvoets the changes requested have been made. Thanks |
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.
Looks good, some minor comments
|
||
public RestructureAvroRecords(String inputPath, String outputPath) { | ||
this.setInputWebHdfsURL(inputPath); | ||
private RestructureAvroRecords(String hdfsUri, String outputPath, boolean gzip, boolean dedup, String format) { |
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.
You can pass the Builder class here instead, to prevent large argument lists.
return this; | ||
} | ||
|
||
public Builder doDeuplicate(final boolean dedup) { |
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.
doDeuplicate -> doDeduplicate
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.
Thanks, LGTM
Add a command line parser for all the options
closes #22
The following options are present -