Support 0 for model.Duration. #226
Conversation
This allows to pass the string "0" to parse a model.Duration. This is because `time.Duration` also support this zero value and so it is to interchange model.Duration with time.Duration.
time.Duration does parses 0 using this:
```
// Special case: if all that is left is "0", this is zero.
if s == "0" {
return 0, nil
}
```
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
|
We always require a non-zero value when this is used in Prometheus, so I think requiring a unit is reasonable. |
|
Ok ! |
This is not true, we can now set the retention duration as |
I'd argue that that's not a sane setup, as you'll still have 2 hours in the head. |
|
Nope, this is when I want only size based retention, and not time based. The way to disable time based is to set it to 0, if I'm not wrong? |
|
That's true. |
model/time.go
Outdated
| @@ -186,6 +186,10 @@ var durationRE = regexp.MustCompile("^([0-9]+)(y|w|d|h|m|s|ms)$") | |||
| // ParseDuration parses a string into a time.Duration, assuming that a year | |||
| // always has 365d, a week always has 7d, and a day always has 24h. | |||
| func ParseDuration(durationStr string) (Duration, error) { | |||
| // shortcut if the value is 0 | |||
There was a problem hiding this comment.
This implies that this is a fast path, which it isn't.
There was a problem hiding this comment.
Why isn't it a fast path? If I don't have to spin up the regexp machinery, I'd totally call it a fast path.
In different news: Should we accept "-0"? https://golang.org/pkg/time/#ParseDuration does, BTW…
In yet different news, comment should start capitalized and end with a period.
There was a problem hiding this comment.
I guess what you might refer to is that "0" would not pass the code below, and thus it's not only fast, but also handling a special case. (In general, explaining your thoughts a tad more detailed might help…)
There was a problem hiding this comment.
I'll improve this comment should we also support "-0" ? You tell me.
There was a problem hiding this comment.
Thinking about it: We do not support negative durations in general, so we should not support "-0" either.
|
Note: https://golang.org/pkg/time/#ParseDuration also doesn't need a unit iff the value is "0". |
Yeah that's what I was trying to explain in the PR description. |
|
You did @cyriltovena . I have to apologize for my limited attention span. (Pretending to be a millennial… :o) |
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
model/time.go
Outdated
| @@ -186,7 +186,7 @@ var durationRE = regexp.MustCompile("^([0-9]+)(y|w|d|h|m|s|ms)$") | |||
| // ParseDuration parses a string into a time.Duration, assuming that a year | |||
| // always has 365d, a week always has 7d, and a day always has 24h. | |||
| func ParseDuration(durationStr string) (Duration, error) { | |||
| // shortcut if the value is 0 | |||
| // Special case: if we receive a "0" string we can directly return a zero value without further processing. | |||
There was a problem hiding this comment.
This is a bit extreme, just "Allow 0 without a unit." would do.
|
Can you add the DCO? |
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
cyriltovena
force-pushed
the
default-mode-duration
branch
from
fc259e5 to
83be373
Compare
|
Everyone seems happy now. Merging… |
Migrate from go-kit/kit/log to go-kit/log
This allows to pass the string "0" to parse a model.Duration. This is because
time.Durationalso support this zero value and so it is easier to interchange model.Duration with time.Duration.time.Duration does parses 0 using this:
see https://golang.org/src/time/format.go?s=40541:40587#L1389
Signed-off-by: Cyril Tovena cyril.tovena@gmail.com