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

Rename command-line option --expiration-date #4931

Closed
dchristidis opened this issue Oct 21, 2021 · 6 comments · Fixed by #4938
Closed

Rename command-line option --expiration-date #4931

dchristidis opened this issue Oct 21, 2021 · 6 comments · Fixed by #4938

Comments

@dchristidis
Copy link
Contributor

dchristidis commented Oct 21, 2021

Motivation

From the documentation of rucio-admin replicas declare-temporary-unavailable:

  --expiration-date EXPIRATION_DATE
                        Timeout in hours when the replicas will become available again. Default 24

This option name is misleading as the argument is not in date format. This has lead to some case were people tried something like --expiration-date 20210720, which works, but the expiration date set in Rucio is in year 4327.

I think it’s preferable to maintain a relative timeout (like with rule lifetime) rather than enforcing some date format.

Modification

  1. Add a new option named something like --expiration-timeout or --duration and deprecate the current one, with the intention of removing it entirely in the next major Rucio release.
  2. Use seconds instead of hours to match update-rule --lifetime.
  3. Consider removing the default value (the user should set it explicitly).
  4. Update the documentation.
  5. Have an allowed range, maybe at least one day but less that a month? 0 should be allowed as an exception and would mean ‘revert the previous declaration’. This might have to be implemented server-side too (if so, a different issue is needed).
@joeldierkes
Copy link
Contributor

Some thoughts:

  1. & 2. Explicitly add --duration-in-hours and --duration-in-seconds to be more specific. Users are used to hours and this could lead to confusion.
  2. We should consider adding another CLI option like --abort-previsous-declaration rather than overloading some numbers. This would be more explicit and more user-intuitive.

joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 25, 2021
The command line option `--expiration-date` is missleading. The option defines a
duration in hours, where as the name indicates a date.
@dchristidis
Copy link
Contributor Author

An interesting remark, but I think would rather have the CLI use seconds as a default and be able to process suffixes like h, d, m and y (this would be useful for add-rule and update-rule too).

@joeldierkes
Copy link
Contributor

I like the idea of doing it consistent and having more than just seconds. However, just defining our own norm could also be annoying for users. We should create an extra issue for that. I'll go with seconds now.

@dchristidis
Copy link
Contributor Author

This feature is rarely used and probably unknown to most; I don’t regard changing the unit (as long as it’s accompanied by changing the option name) as a concern.

joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 25, 2021
The command line option `--expiration-date` is missleading. The option defines a
duration in hours, where as the name indicates a date.
joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 25, 2021
The command line option `--expiration-date` is missleading. The option defines a
duration in hours, where as the name indicates a date.
bari12 added a commit that referenced this issue Oct 26, 2021
…_line_option___expiration_date

Client: Rename command-line option --expiration-date Fix #4931
@dchristidis
Copy link
Contributor Author

Given that item 5 remains to be discussed and implemented, I would suggest reopening the issue.

@bari12
Copy link
Member

bari12 commented Oct 27, 2021

@dchristidis or @joeldierkes please create a ticket for this separately. That range should be implemented on server side.

joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 28, 2021
As referenced in rucio#4931 the CLI option `--duration` of `rucio-admin replicas
declare-temporary-unavailable` should rather have an allowed time-range than
accepting every number. This avoids the mistake of submitting time-ranges that
are too big (e.g. > 1000y).
joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 28, 2021
As referenced in rucio#4931 the CLI option `--duration` of `rucio-admin replicas
declare-temporary-unavailable` should rather have an allowed time-range than
accepting every number. This avoids the mistake of submitting time-ranges that
are too big (e.g. > 1000y).
joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 28, 2021
As referenced in rucio#4931 the CLI option `--duration` of `rucio-admin replicas
declare-temporary-unavailable` should rather have an allowed time-range than
accepting every number. This avoids the mistake of submitting time-ranges that
are too big (e.g. > 1000y).
joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 28, 2021
As referenced in rucio#4931 the CLI option `--duration` of `rucio-admin replicas
declare-temporary-unavailable` should rather have an allowed time-range than
accepting every number. This avoids the mistake of submitting time-ranges that
are too big (e.g. > 1000y).
joeldierkes pushed a commit to joeldierkes/rucio that referenced this issue Oct 28, 2021
As referenced in rucio#4931 the CLI option `--duration` of `rucio-admin replicas
declare-temporary-unavailable` should rather have an allowed time-range than
accepting every number. This avoids the mistake of submitting time-ranges that
are too big (e.g. > 1000y).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants