Retention: switch number of days type uint16 -> float64 #883
Conversation
Al2Klimov
force-pushed
the
retention-subdays
branch
from
568697e to
c1eae5b
Compare
| olderThan := tick.Time | ||
| wholeDays, dayFraction := math.Modf(float64(days)) | ||
|
|
||
| if wholeDays > 0 { |
There was a problem hiding this comment.
Can this be negative? Above in line 171/172 it is already checked if days < 1.
Btw, you may want to change the abort constraint to days < 0 as otherwise you cannot use half days, right?
There was a problem hiding this comment.
Can this be negative?
I guess you answered your own question:
Above in line 171/172 it is already checked if
days < 1.
So no, it can't be.
(If I understand you correctly.)
julianbrost
left a comment
julianbrost
left a comment
There was a problem hiding this comment.
If I remember correctly, that config attribute only exists in this way instead of being a standard time.Duration because the largest unit time.ParseDuration is hours and you'd have to configure large non-obvious values for realistic use-cases. An alternative could be to allow more advanced duration strings like for example history: 1y, then the change would also provide some real-world improvement.
And another thing to consider: could allowing very small values conflict with writing the history? Like could this trigger foreign key errors because the referenced row was already deleted? The minimum of 1 day might actually provide some safety margin there.
| olderThan := tick.Time | ||
| wholeDays, dayFraction := math.Modf(float64(days)) | ||
|
|
||
| if wholeDays > 0 { | ||
| olderThan = olderThan.AddDate(0, 0, -int(wholeDays)) | ||
| } | ||
|
|
||
| if dayFraction > 0 { | ||
| olderThan = olderThan.Add(-time.Duration(dayFraction * float64(24*time.Hour))) | ||
| } |
There was a problem hiding this comment.
Does this actually do something more useful than just days * 24 * time.Hour? Yes, there's a slight difference with DST changes, but does it even do something useful then?
There was a problem hiding this comment.
IIRC, only a limited number of days fit into time.Duration. I'm working around that limitation here.
E.g 1h safety margin would also do it. |
Al2Klimov
force-pushed
the
retention-subdays
branch
from
c1eae5b to
b769675
Compare
|
BUT then I'd tell users to increase the threshold. |
Al2Klimov
force-pushed
the
retention-subdays
branch
from
b769675 to
8cce69c
Compare
This allows specifying less than a day as threshold. That is especially useful if history doesn't mattter as much as disk space.
Al2Klimov
force-pushed
the
retention-subdays
branch
from
8cce69c to
2a42460
Compare
oxzi
changed the title
|
Allowing fractions for days seems wrong. While I understand the use case, I would suggest not misusing However, before starting to implement such a change, some other opinions may be useful. |
I wouldn't change anything and just close this PR until we have a specific feature request. |
|
Thanks for your second opinion. As having a retention period of less than a day is more a development than a production requirement and that the config either "feels" wrong or requires a breaking change, I would close this now. Thanks for all your work, @Al2Klimov. If the needs arises, we can resurrect this PR. |
This allows specifying less than a day as threshold. That is especially useful if history doesn't mattter as much as disk space.
I want my large (ref/NC/820479) test setup to run over WE w/o running out of disk space. So I built this.