compactor: adjust interval for period <1-hour #9485
Conversation
|
/cc @yudai @fanminshi |
compactor/periodic.go
Outdated
|
|
||
| plog.Noticef("Starting auto-compaction at revision %d (retention: %v)", rev, t.period) | ||
| _, err := t.c.Compact(t.ctx, &pb.CompactionRequest{Revision: rev}) | ||
| if err == nil || err == mvcc.ErrCompacted { | ||
| // move to next sliding window | ||
| t.revs = remaining | ||
| t.revs = t.revs[1:] |
There was a problem hiding this comment.
is this necessary?
the t.revs will be updated at line 110 right?
compactor/periodic.go
Outdated
| interval := t.period / time.Duration(periodDivisor) | ||
| compactInterval := t.getCompactInterval() | ||
| retryInterval := t.getRetryInterval() | ||
| retentions := int(t.period/compactInterval) + 1 // number of revs to keep for t.period |
There was a problem hiding this comment.
is retentions always equal to retryDivisor in this case?
There was a problem hiding this comment.
actually, it doesn't really matter. just ignore what I said.
| @@ -25,7 +25,7 @@ import ( | |||
| "github.com/jonboulle/clockwork" | |||
| ) | |||
|
|
|||
| func TestPeriodic(t *testing.T) { | |||
| func TestPeriodicHourly(t *testing.T) { | |||
There was a problem hiding this comment.
we can just follow testing strategy from this
https://github.com/coreos/etcd/blob/f65aee0759633966e0e1052d0c18079da551d24e/compactor/periodic_test.go#L27-L72
There was a problem hiding this comment.
we probably adjust fc.Advance(checkCompactionInterval) to fc.Advance(6*time.Minute) .
compactor/periodic_test.go
Outdated
| // simulate 115 (23 * 5) minutes | ||
| for i := 0; i < 5; i++ { | ||
| rg.Wait(1) | ||
| fc.Advance(retentionDuration) |
There was a problem hiding this comment.
we should call fc.Advance() once per retry interval so that the compactor can update it's revs array with more entries. The above code fc.Advance(retentionDuration) only let compactor to get rev once; hence len(revs)==1 which is not what it should have for a given retentionDuration.
There was a problem hiding this comment.
see how the old test code work from the above link.
|
Overall LGTM besides the commend from @fanminshi about the extra window sliding. |
gyuho
force-pushed
the
compactor
branch
2 times, most recently
from
da9f14e to
377cf6b
Compare
|
@gyuho can you try to set compaction time to a low value: say 30s, 1min, 5min and 15min to verify if the behavior is reasonable? |
compactor/periodic.go
Outdated
| } | ||
|
|
||
| func (t *Periodic) getRetentions() int { | ||
| return int(t.period / t.getRetryInterval()) |
There was a problem hiding this comment.
shouldn't this be int(t.period / t.getRetryInterval()) + 1?
compactor/periodic_test.go
Outdated
|
|
||
| n1 := tb.getRetentions() | ||
|
|
||
| // compaction doesn't happen til 5 hours elapses |
compactor/periodic_test.go
Outdated
| // compaction happens at every interval | ||
| for i := 0; i < 5; i++ { | ||
| // advance 5-minute, one revision for each interval | ||
| for j := 0; j < 10; j++ { |
There was a problem hiding this comment.
don't use hard code the value 10. use a variable instead.
maybe intervalsPerPeriod?
compactor/periodic_test.go
Outdated
| t.Fatal(err) | ||
| } | ||
|
|
||
| expectedRevision = int64((i+1)*10 + 1) |
There was a problem hiding this comment.
don't use hard coded value 10. use some like intervalsPerPeriod instead.
compactor/periodic_test.go
Outdated
| // now compactor kicks in, every hour | ||
| for i := 0; i < 3; i++ { | ||
| // advance one hour, one revision for each interval | ||
| for j := 0; j < 10; j++ { |
There was a problem hiding this comment.
same here. use something like intervalsPerPeriod instead.
compactor/periodic_test.go
Outdated
| n1 := tb.getRetentions() | ||
|
|
||
| // compaction doesn't happen til 2 hours elapses | ||
| for i := 0; i < n1; i++ { |
There was a problem hiding this comment.
better if you can explain what n1 is.
compactor/periodic_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| expectedRevision := int64(i + 1 - n) | ||
|
|
||
| expectedRevision = int64((i+1)*10 + 1) |
There was a problem hiding this comment.
don't use hard code 10. use something like intervalsPerPeriod instead.
|
lgtm after nits. |
|
All addressed. Manually tested some intervals, and the retention window seems reasonable. For instance, with compaction period 30s and write rate is 1 req/s. First compaction runs at [T=30s revision=1]. Second happens at [T=60s revision=30]. So 30-second retention window is maintained. |
|
lgtm if test passes |
Signed-off-by: Gyuho Lee <[email protected]>
Simplified version of #9482.
Similar to how v3.2 works while handling compaction period <1-hour.
Address #9443.