Skip to content
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

feat: Added an ability to pass a replication count via the CLI #169

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion hdfs/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
Usage:
hdfscli [interactive] [-a ALIAS] [-v...]
hdfscli download [-fsa ALIAS] [-v...] [-t THREADS] HDFS_PATH LOCAL_PATH
hdfscli upload [-sa ALIAS] [-v...] [-A | -f] [-t THREADS] LOCAL_PATH HDFS_PATH
hdfscli upload [-sa ALIAS] [-v...] [-A | -f] [-r REPLICATION] [-t THREADS] LOCAL_PATH HDFS_PATH
hdfscli -L | -V | -h

Commands:
Expand Down Expand Up @@ -36,12 +36,14 @@
0 allocates a thread per file. [default: 0]
-v --verbose Enable log output. Can be specified up to three
times (increasing verbosity each time).
-r REPLICATION --replication=REPLICATION Set the expected HDFS replication count [default: 3]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's shorten the name to fit within the left hand side. For example:

-r COUNT --replication=COUNT

Can you also move it before -s to keep the options sorted?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code doesn't set the default. Can you keep this behavior when the option isn't set? The cluster might override the default site-wide.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed your reply and switched to other stuff. :-(

The current code doesn't set the default.

I'm super confused with that docs-as-a-config thing, but as I understand, [default: 3] sets the defaults?

Can you keep this behavior when the option isn't set? The cluster might override the default site-wide.

Not sure I follow. The cluster does have site-wide settings but the client can override them. So if you don't pass that fro the client perspective - cluster defaults will be used.

Copy link
Owner

@mtth mtth Nov 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code (before this PR) doesn't set the default on the client, so the cluster's default will be used. This is a nice property which would be lost after this change since the client now sets a default of 3. Instead the client should only set replication when explicitly specified by the user.


Examples:
hdfscli -a prod /user/foo
hdfscli download features.avro dat/
hdfscli download logs/1987-03-23 - >>logs
hdfscli upload -f - data/weights.tsv <weights.tsv
hdfscli upload -f -r 1 - data/weights.tsv <weights.tsv

HdfsCLI exits with return status 1 if an error occurred and 0 otherwise.

Expand Down Expand Up @@ -201,6 +203,7 @@ def main(argv=None, client=None):
hdfs_path = args['HDFS_PATH']
local_path = args['LOCAL_PATH']
n_threads = parse_arg(args, '--threads', int)
replication_n = parse_arg(args, '--replication', int)
force = args['--force']
silent = args['--silent']
if args['download']:
Expand Down Expand Up @@ -240,6 +243,7 @@ def main(argv=None, client=None):
(line for line in sys.stdin), # Doesn't work with stdin.
append=append,
overwrite=force,
replication=replication_n,
)
else:
if append:
Expand All @@ -260,6 +264,7 @@ def main(argv=None, client=None):
overwrite=force,
n_threads=n_threads,
progress=progress,
replication=replication_n,
)
else:
banner = (
Expand Down