-
Notifications
You must be signed in to change notification settings - Fork 103
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
base: master
Are you sure you want to change the base?
Conversation
Hm, the CI errors seems unrelated to the PR:
|
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.
Apologies for the delayed review @Oloremo. Two minor comments and this should be good to go.
@@ -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] |
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.
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?
@@ -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] |
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.
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.
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.
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.
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.
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.
Just adding an ability to pass a replication count for upload, default is preserved.