-
Notifications
You must be signed in to change notification settings - Fork 107
make index pruning configurable via index-rules.conf #924
Conversation
a39085a
to
07a49fb
Compare
It seems like we should just add the |
maybe. i've been going back and forth on that a bit myself. the benefit of separate file is that it easily allows to add more per-pattern index tunables in the future. though i don't know yet which those would be. eg schemas:
index rules:
becomes:
while for all the simple cases (and the majority of our deployments are simple) the extra file approach makes things a bit harder, having it all in 1 file makes the complication of more complicated cases a multiple. and that's a bit concerning in terms of operability. the benefit of single file is we save some memory by not having to track the separate IrId. I don't see a strong reason to go either way though. |
ok, lets stick with the separate file for now. |
conf/indexrules.go
Outdated
} | ||
|
||
type IndexCheck struct { | ||
Keep bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need the Keep
field. Cant we just set Cutoff
to 0 if maxStale==0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
# * Valid units are s/sec/secs/second/seconds, m/min/mins/minute/minutes, h/hour/hours, d/day/days, w/week/weeks, mon/month/months, y/year/years | ||
|
||
[default] | ||
pattern = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have a pattern set? eg .*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed. empty string is a valid regex that matches everything. (the first test in conf/indexrules_test.go
checks this)
idx/cassandra/cassandra.go
Outdated
staleTs := time.Now().Add(maxStale * -1) | ||
_, err := c.Prune(staleTs) | ||
for now := range ticker.C { | ||
log.Debug("cassandra-idx: pruning items") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have an info level log message for when a prune starts, and an info message when the prune completes. The message on completion should log how long the prune took and number of items pruned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have such info logs for the memory index and the cassandra index doesn't really do any work beyond that, but I think it's still useful. in fact i'm making the logs from both more consistent so that it'll look obvious in the logs, both memory and cassandra index logging their own steps. in the future the cassandra-idx may do its own extra steps again.
idx/memory/memory.go
Outdated
@@ -1251,9 +1269,13 @@ func (m *MemoryIdx) Prune(oldest time.Time) ([]idx.Archive, error) { | |||
pre := time.Now() | |||
|
|||
m.RLock() | |||
|
|||
// getting all checks once saves having to recompute the cutoff everytime we have a match | |||
indexChecks := IndexRules.Checks(now) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move this to before we acquire the RLock(). We want to keep lock times as small as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops
# * Valid units are s/sec/secs/second/seconds, m/min/mins/minute/minutes, h/hour/hours, d/day/days, w/week/weeks, mon/month/months, y/year/years | ||
|
||
[default] | ||
pattern = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have a match-all pattern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty string is a valid regex that matches everything. (the first test in conf/indexrules_test.go checks this)
PTAL |
conf/indexrules_test.go
Outdated
}, | ||
} | ||
for i, c := range cases { | ||
err := ioutil.WriteFile("/tmp/indexrules-test-readindexrules", []byte(c.in), 0644) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather then a hard coded filename, you should use ioutil.TempFile()
https://golang.org/pkg/io/ioutil/#TempFile
You also need to make sure you delete the file when done.
note that CassandraIdx.load() returns results out of order. No idea why our tests didn't fail before this but now we correctly work around this.
rebased on master |
for #868