Support setting replication level at destination in arv-copy options #247
Conversation
|
A minor point I should add -- when encountering an invalid replication level (such as zero or a negative number), it behaves as if the command line option was not set, and will go on to use the default value obtained from the dst cluster config. |
arv-copy effectively used a hard-coded replication level 2 for the copied collections at the destination, bypassing the default replication level set in the destination cluster's configuration file. There was no command-line option to override this behavior. A new command-line option, --replication, is added to the arv-copy command, following the arv-put command's semantics. If left unspecified, the destination's default replication setting is used. If that setting cannot be found, use the fallback value of 2. Arvados-DCO-1.1-Signed-off-by: Zoë Ma <[email protected]>
zoe-translates
force-pushed
the
arv-copy-respect-destination-cluster-default-replication-setting-and-add-cmdline-option-to-override
branch
from
bc1a005
to
82c9300
Compare
|
| @@ -111,6 111,9 @@ def main(): | |||
| copy_opts.add_argument( | |||
| '--project-uuid', dest='project_uuid', | |||
| help='The UUID of the project at the destination to which the collection or workflow should be copied.') | |||
| copy_opts.add_argument( | |||
| '--replication', type=int, metavar='N', | |||
There was a problem hiding this comment.
We can pass a custom type function that validates that the value is a positive integer, and raises ValueError if not. With this change, argparse would immediately report the error to the user, which provides a nicer UI than an easy-to-miss debug message.
arvados.commands._util._pos_int is an example of a custom type function, and it almost does what you want, except it accepts 0 as a valid value. We could turn that function into a closure that accepts a minimum allowable value, and then use it here in addition to retry_opt below.
| else: | ||
| # Obtain default or fallback collection replication setting on the | ||
| # destination | ||
| n_copies = dst.config().get("Collections", {}).get("DefaultReplication") |
There was a problem hiding this comment.
Consider instead:
try:
n_copies = dst.config()["Collections"]["DefaultReplication"]
except KeyError:
n_copies = 2This way, instead of interleaving default logic and lookup logic, you can just do the lookup you prefer, and have a fallback in case it fails at any point. (It should also perform better, but for sure that basically doesn't matter for a one-time lookup outside a hot loop like this.)
arv-copy effectively used a hard-coded replication level 2 for the copied collections at the destination, bypassing the default replication level set in the destination cluster's configuration file. There was no command-line option to override this behavior.
A new command-line option, --replication, is added to the arv-copy command, following the arv-put command's semantics. If left unspecified, the destination's default replication setting is used. If that setting cannot be found, use the fallback value of 2.
Arvados-DCO-1.1-Signed-off-by: Zoë Ma [email protected]