Skip to content

feat: Add Duration::from_minutes, hours and days #657

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

Merged
merged 7 commits into from
Oct 9, 2023

Conversation

sbernauer
Copy link
Member

Description

Please add a description here. This will become the commit message of the merge request later.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes
# Author
- [ ] Changes are OpenShift compatible
- [ ] CRD changes approved
- [ ] Integration tests passed (for non trivial changes)
# Reviewer
- [ ] Code contains useful comments
- [ ] (Integration-)Test cases added
- [ ] Documentation added or updated
- [ ] Changelog updated
- [ ] Cargo.toml only contains references to git tags (not specific commits or branches)
# Acceptance
- [ ] Feature Tracker has been updated
- [ ] Proper release label has been added

@sbernauer
Copy link
Member Author

@Techassi @nightkr so, what is the plan here? I see the following options

  1. Duration::from_secs(15 * 24 * 60 * 60) in operator code
  2. const fn Duration::from_days (this PR)
  3. Some macro solution, e.g. duration!("2d")

Both 2. and 3. would work for me.

I don't have enough Rust const knowledge, but my understanding is that Option 2. comes with a panic!() in case the Duration overflows. This is the same way time::Duration::days(days: i64) https://docs.rs/time/0.3.29/src/time/duration.rs.html#448 works and what we currently use e.g. in secret-op.

What would be your preferred Option?

@Techassi
Copy link
Member

Techassi commented Sep 25, 2023

  1. Duration::from_secs(15 * 24 * 60 * 60) in operator code

I agree that this is quite tedious.

  1. const fn Duration::from_days (this PR)

A panic is necessary, because Result cannot be used in constant environments (and it wouldn't make sense if it could). std::time::Duration will also panic when the internal u64 would overflow. I don't have any strong opinions on re-panicing in our code.

  1. Some macro solution, e.g. duration!("2d")

This would need some more research (I would volunteer for that if we decide to move forward with it). Additionally, we have to maintain the macro code. This sometimes can get a little tricky, but this macro would probably be easy enough to maintain.


We can also choose to move forward using a "middle-ground" solution like exporting public constants like:

pub const ONE_MINUTE_IN_SECS: u64 = 60;
pub const ONE_HOUR_IN_SECS: u64 = ONE_MINUTE_IN_SECS * 60;
pub const ONE_DAY_IN_SECS: u64 = ONE_HOUR_IN_SECS * 24;

// In operator code

use stackable_operator::duration::{Duration, ONE_DAY_IN_SECS};

pub const TLS_CERT_LIFETIME = Duration::from_secs(365 * ONE_DAY_IN_SECS);

@nightkr
Copy link
Member

nightkr commented Sep 27, 2023

As I've written before, my primary concern is that making it too convenient to write long durations.. encourages people to use more long durations where doing so is largely an antipattern or just puts a band-aid over a bigger issue.

@sbernauer
Copy link
Member Author

I don't think the time periods are going to change based upon the presence or absence of an Duration::from_XXX function.
And e.g. Duration::from_secs(6 * 60 * 60) is just less readable and error prone than Duration::from_hours(6) (and 6 hours is a total legit time period for e.g. max query runtime, graceful shutdown period, scale down time period and much more)

@lfrancke
Copy link
Member

lfrancke commented Oct 4, 2023

During todays architecture meeting we decided to go ahead with this feature (not necessarily this implementation).
We also decided that it makes more sense to add warnings at the time where this is used.
e.g. the operators can warn if a user uses a value that we consider too high.

@sbernauer
Copy link
Member Author

@siegfriedweber, @Techassi and I did go though the PR and reached a conclusion.
User input is handled with the Serialize function and is not handled by the code in this PR. The functions of this PR are now prefixed with _unchecked and are only meant for constants.
We don't want to merge this straight up, but give @nightkr and @fhennig a chance to comment or approve.

@sbernauer sbernauer force-pushed the feat/add-duration-from-consts branch from b2ccbbd to fd0b0d4 Compare October 9, 2023 07:40
@sbernauer sbernauer enabled auto-merge October 9, 2023 09:46
@sbernauer sbernauer added this pull request to the merge queue Oct 9, 2023
Merged via the queue into main with commit 5a26e35 Oct 9, 2023
@sbernauer sbernauer deleted the feat/add-duration-from-consts branch October 9, 2023 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants