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.

Sign up for GitHub

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

Retention: switch number of days type uint16 -> float32 #883

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Retention: switch number of days type uint16 -> float32 #883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ type Flags struct {

// RetentionConfig defines configuration for history retention.
type RetentionConfig struct {
HistoryDays uint16 `yaml:"history-days"`
SlaDays uint16 `yaml:"sla-days"`
HistoryDays float64 `yaml:"history-days"`
SlaDays float64 `yaml:"sla-days"`
Interval time.Duration `yaml:"interval" default:"1h"`
Count uint64 `yaml:"count" default:"5000"`
Options history.RetentionOptions `yaml:"options"`
Expand Down
26 changes: 18 additions & 8 deletions pkg/icingadb/history/retention.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/icinga/icingadb/pkg/icingaredis/telemetry"
"github.com/pkg/errors"
"go.uber.org/zap"
"math"
"time"
)

Expand Down Expand Up @@ -95,7 +96,7 @@ var RetentionStatements = []retentionStatement{{
}}

// RetentionOptions defines the non-default mapping of history categories with their retention period in days.
type RetentionOptions map[string]uint16
type RetentionOptions map[string]float64

// Validate checks constraints in the supplied retention options and
// returns an error if they are violated.
Expand All @@ -120,16 +121,16 @@ func (o RetentionOptions) Validate() error {
type Retention struct {
db *database.DB
logger *logging.Logger
historyDays uint16
slaDays uint16
historyDays float64
slaDays float64
interval time.Duration
count uint64
options RetentionOptions
}

// NewRetention returns a new Retention.
func NewRetention(
db *database.DB, historyDays, slaDays uint16, interval time.Duration,
db *database.DB, historyDays, slaDays float64, interval time.Duration,
count uint64, options RetentionOptions, logger *logging.Logger,
) *Retention {
return &Retention{
Expand All @@ -156,7 +157,7 @@ func (r *Retention) Start(ctx context.Context) error {
errs := make(chan error, 1)

for _, stmt := range RetentionStatements {
var days uint16
var days float64
switch stmt.RetentionType {
case RetentionHistory:
if d, ok := r.options[stmt.Category]; ok {
Expand All @@ -168,7 +169,7 @@ func (r *Retention) Start(ctx context.Context) error {
days = r.slaDays
}

if days < 1 {
if days <= 0 {
r.logger.Debugf("Skipping history retention for category %s", stmt.Category)
continue
}
Expand All @@ -177,12 +178,21 @@ func (r *Retention) Start(ctx context.Context) error {
fmt.Sprintf("Starting history retention for category %s", stmt.Category),
zap.Uint64("count", r.count),
zap.Duration("interval", r.interval),
zap.Uint16("retention-days", days),
zap.Float64("retention-days", days),
)

stmt := stmt
periodic.Start(ctx, r.interval, func(tick periodic.Tick) {
olderThan := tick.Time.AddDate(0, 0, -int(days))
olderThan := tick.Time
wholeDays, dayFraction := math.Modf(float64(days))

if wholeDays > 0 {
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.)

olderThan = olderThan.AddDate(0, 0, -int(wholeDays))
}

if dayFraction > 0 {
olderThan = olderThan.Add(-time.Duration(dayFraction * float64(24*time.Hour)))
}
Comment on lines +186 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIRC, only a limited number of days fit into time.Duration. I'm working around that limitation here.


r.logger.Debugf("Cleaning up historical data for category %s from table %s older than %s",
stmt.Category, stmt.Table, olderThan)
Expand Down
Loading