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: Add support for human-readable duration formats #647

Merged
merged 38 commits into from
Sep 18, 2023
Merged

Conversation

Techassi
Copy link
Member

@Techassi Techassi commented Sep 7, 2023

Fixes #649. This was discussed in an arch meeting roughly two weeks ago (when this PR was opened).

It is part of the effort to unify duration formats used in our CRDs. Operators would need to use the exported Duration type to support human-readable duration formats. This PR adds support for human-readable durations, like 2d2h20m42s or15d2m2s. The Duration type requirements are tracked in #649

These changes are breaking, because:

  • in some cases the type changes from number to string
  • in cases where string is already used, the parsing might be slightly different

Author

Preview Give feedback

Reviewer

Preview Give feedback

@Techassi Techassi self-assigned this Sep 7, 2023
@Techassi Techassi changed the title Feat/humantime feat: Add support for human-readable duration formats Sep 7, 2023
@Techassi
Copy link
Member Author

Techassi commented Sep 8, 2023

In its current form, the Duration is not usable in const environments, thus we are unable to define constant values like here: https://github.com/stackabletech/trino-operator/blob/main/rust/crd/src/lib.rs#L124

There currently is an open issue upstream in humantime, but there seems little to no progress. We could, however, implement our own (simplified) parsing.

@Techassi
Copy link
Member Author

Techassi commented Sep 13, 2023

Constant values are now possible:

use stackable_operator::duration::Duration;

const DEFAULT_TIMEOUT: Duration = Duration::from_secs(3600);

@Techassi Techassi marked this pull request as ready for review September 13, 2023 11:14
Techassi and others added 4 commits September 14, 2023 14:09
This reworks/simplifies the `Duration` implementation. Supported
units are now clearly defned by an enum and the parsing/display
mechanism is now way more explicit and defined in one place.

Co-authored-by: Natalie Klestrup Röijezon <teo.roijezon@stackable.de>
Co-authored-by: Natalie Klestrup Röijezon <teo.roijezon@stackable.de>
Co-authored-by: Natalie Klestrup Röijezon <teo.roijezon@stackable.de>
We need to check if the iterator progressed based on the start index of
the unit / the end index of the numeric value.
src/duration/mod.rs Outdated Show resolved Hide resolved
src/duration/mod.rs Outdated Show resolved Hide resolved
src/duration/mod.rs Show resolved Hide resolved
src/duration/mod.rs Outdated Show resolved Hide resolved
src/duration/mod.rs Outdated Show resolved Hide resolved
src/duration/mod.rs Outdated Show resolved Hide resolved
This commit now parses durations in the Go format. This format
doesn't use whitespaces to separate each fragment. Instead, all
fragments are concated without any space between. The parser needs
to be a little cleverer when trying to keep it streamlined.

Additionally, the error variants now contain some more context to
make it easier for the user to spot the error in the provided value.

Co-authored-by: Natalie Klestrup Röijezon <teo.roijezon@stackable.de>
@Techassi Techassi requested a review from nightkr September 15, 2023 07:53
src/duration/mod.rs Outdated Show resolved Hide resolved
@Techassi
Copy link
Member Author

Commit fe1911e breaks duration::test::parse_invalid::case_3. This change needs to be reverted.

@Techassi Techassi requested a review from sbernauer September 18, 2023 08:59
@sbernauer
Copy link
Member

Actual change LGTM, giving it a try with trino-op graceful shutdown

@sbernauer
Copy link
Member

error[E0277]: cannot multiply `{integer}` by `stackable_operator::duration::Duration`
  --> rust/operator-binary/src/operations/graceful_shutdown.rs:46:13
   |
46 |         + 2 * GRACEFUL_SHUTDOWN_GRACE_PERIOD
   |             ^ no implementation for `{integer} * stackable_operator::duration::Duration`
   |
   = help: the trait `Mul<stackable_operator::duration::Duration>` is not implemented for `{integer}`

@sbernauer
Copy link
Member

Looking at the generated deploy/helm/trino-operator/crds/crds.yaml, we have the following change I would have not expected.

                     gracefulShutdownTimeout:
                       default: 1h
                       description: Time period the trino workers have to gracefully shut down, e.g. `1h`, `30m` or `2d`. Consult the trino-operator documentation for details.
-                      type: string
+                      properties:
+                        nanos:
+                          format: uint32
+                          minimum: 0.0
+                          type: integer
+                        secs:
+                          format: uint64
+                          minimum: 0.0
+                          type: integer
+                      required:
+                        - nanos
+                        - secs
+                      type: object

@sbernauer
Copy link
Member

@Techassi as discussed, pushed the fixes, please have a look

sbernauer
sbernauer previously approved these changes Sep 18, 2023
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

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

Tested and works fine for the trino graceful shutdown, awesome!
However, I would give @nightkr the chance to review the code again

@Techassi
Copy link
Member Author

Yes, looks good! Waiting for final feedback from @nightkr

@nightkr
Copy link
Member

nightkr commented Sep 18, 2023

Commit fe1911e breaks duration::test::parse_invalid::case_3. This change needs to be reverted.

Fair, I should have fixed those test cases.

src/duration/mod.rs Outdated Show resolved Hide resolved
@nightkr
Copy link
Member

nightkr commented Sep 18, 2023

Commit fe1911e breaks duration::test::parse_invalid::case_3. This change needs to be reverted.

Fair, I should have fixed those test cases.

98149d7

@Techassi Techassi added this pull request to the merge queue Sep 18, 2023
Merged via the queue into main with commit 9c020d4 Sep 18, 2023
16 checks passed
@Techassi Techassi deleted the feat/humantime branch September 18, 2023 12:11
@sbernauer
Copy link
Member

Awesome, thanks for working on this!

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.

Tracking: Support human-readable duration format in CRDs
3 participants