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.

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

compactor: adjust interval for period <1-hour #9485

Merged
merged 2 commits into from
Mar 26, 2018
Merged

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Mar 23, 2018

Simplified version of #9482.
Similar to how v3.2 works while handling compaction period <1-hour.

Address #9443.

@gyuho
Copy link
Contributor Author

gyuho commented Mar 23, 2018

/cc @yudai @fanminshi


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:]
Copy link
Member

@fanminshi fanminshi Mar 23, 2018

Choose a reason for hiding this comment

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

is this necessary?
the t.revs will be updated at line 110 right?

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
Copy link
Member

Choose a reason for hiding this comment

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

is retentions always equal to retryDivisor in this case?

Copy link
Member

@fanminshi fanminshi Mar 23, 2018

Choose a reason for hiding this comment

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

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) {
Copy link
Member

@fanminshi fanminshi Mar 23, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

we probably adjust fc.Advance(checkCompactionInterval) to fc.Advance(6*time.Minute) .

// simulate 115 (23 * 5) minutes
for i := 0; i < 5; i++ {
rg.Wait(1)
fc.Advance(retentionDuration)
Copy link
Member

@fanminshi fanminshi Mar 23, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Member

@fanminshi fanminshi Mar 23, 2018

Choose a reason for hiding this comment

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

see how the old test code work from the above link.

@yudai
Copy link
Contributor

yudai commented Mar 23, 2018

Overall LGTM besides the commend from @fanminshi about the extra window sliding.

@xiang90
Copy link
Contributor

xiang90 commented Mar 23, 2018

@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?

@xiang90 xiang90 closed this Mar 23, 2018
@xiang90 xiang90 reopened this Mar 23, 2018
}

func (t *Periodic) getRetentions() int {
return int(t.period / t.getRetryInterval())
Copy link
Member

@fanminshi fanminshi Mar 23, 2018

Choose a reason for hiding this comment

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

shouldn't this be int(t.period / t.getRetryInterval()) + 1?


n1 := tb.getRetentions()

// compaction doesn't happen til 5 hours elapses
Copy link
Member

Choose a reason for hiding this comment

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

5 hours -> 5 minutes?

// 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++ {
Copy link
Member

Choose a reason for hiding this comment

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

don't use hard code the value 10. use a variable instead.
maybe intervalsPerPeriod?

t.Fatal(err)
}

expectedRevision = int64((i+1)*10 + 1)
Copy link
Member

Choose a reason for hiding this comment

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

don't use hard coded value 10. use some like intervalsPerPeriod instead.

// 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++ {
Copy link
Member

Choose a reason for hiding this comment

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

same here. use something like intervalsPerPeriod instead.

n1 := tb.getRetentions()

// compaction doesn't happen til 2 hours elapses
for i := 0; i < n1; i++ {
Copy link
Member

Choose a reason for hiding this comment

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

better if you can explain what n1 is.

if err != nil {
t.Fatal(err)
}
expectedRevision := int64(i + 1 - n)

expectedRevision = int64((i+1)*10 + 1)
Copy link
Member

Choose a reason for hiding this comment

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

don't use hard code 10. use something like intervalsPerPeriod instead.

@fanminshi
Copy link
Member

lgtm after nits.
also do a manual testing like @xiang90 suggested earlier.

@gyuho
Copy link
Contributor Author

gyuho commented Mar 24, 2018

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.

@xiang90
Copy link
Contributor

xiang90 commented Mar 24, 2018

lgtm if test passes

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants