fix bug in auto-compact #9443

WIZARD-CXY · 2018-03-15T05:43:33Z

we need to update the last variable to record the last compaction time in the for loop. If not we will do compaction in checkCompactInterval instead of t.period . Not quite sure about whether unit test failed to test it out.

WIZARD-CXY · 2018-03-15T05:44:07Z

ping @xiang90

WIZARD-CXY · 2018-03-15T05:44:57Z

work as expected now

bin/etcd --auto-compaction-retention 5s

2018-03-15 13:28:56.373824 N | compactor: Starting auto-compaction at revision 1 (retention: 5s)
2018-03-15 13:28:56.374065 N | compactor: Finished auto-compaction at revision 1
2018-03-15 13:29:01.381026 N | compactor: Starting auto-compaction at revision 1 (retention: 5s)
2018-03-15 13:29:01.381187 N | compactor: Finished auto-compaction at revision 1
2018-03-15 13:29:06.389082 N | compactor: Starting auto-compaction at revision 1 (retention: 5s)
2018-03-15 13:29:06.389224 N | compactor: Finished auto-compaction at revision 1

WIZARD-CXY · 2018-03-15T05:47:21Z

I think this bug is brought by this pr #8563.

xiang90 · 2018-03-15T06:41:22Z

looks reasonable to me. can you add or modify the test to avoid regression in the future?

/cc @gyuho @jpbetz this needs to be back ported.

WIZARD-CXY · 2018-03-15T06:44:00Z

yeah I agree. we need to add test for this

gyuho · 2018-03-15T11:11:08Z

Can you fix test failures? Also, this only needs backport to 3.3.

fanminshi · 2018-03-15T16:58:58Z

compactor/periodic.go

@@ -86,6 +86,10 @@ func (t *Periodic) Run() {
 			}
 			plog.Noticef("Starting auto-compaction at revision %d (retention: %v)", rev, t.period)
 			_, err := t.c.Compact(t.ctx, &pb.CompactionRequest{Revision: rev})
+
+			// update the last compaction time
+			last = clock.Now()


why adding here? should we record last compaction time only when it succeeds?
put it under ?

if err == nil || err == mvcc.ErrCompacted { t.revs = remaining last = clock.Now()

Think it makes sense to reset last for every Compact call, whether it's failed or not.
Otherwise, the interval would be 1/10 of original period, once clock.Now().Sub(last) >= t.period.

@gyuho the bug was introduced here #8563 (diff)
as you can see, the original code sets last only when compaction succeeds.

Yeah saw that. We can just reset last only when it succeeds, and maybe change this later in refactoring.

Actually, when I write this fix, I think it a while.I think we should update the last whether failed or not. when it failed, we don't want to retry faster either(10X faster), otherwise you should write a doc to tell the user.

after second thought, we'd better only update the last when the compact succeeds. Or we have to wait for another compaction duration rather than checkCompactInterval. It also makes the logging at line 98 wrong.

let us fix this as what @fanminshi suggested.

My concern is that I don't want to retry faster(10x faster is too fast) less safe when compact fail.

@WIZARD-CXY if you have a compaction duration, which is 10hours. you 100% do not want to retry after another 10 hours. it means that your etcd db size will likely be doubled.

@xiang90 the thing is compaction duration is 10min in our case, not 10hr

Actually it is all about different user cases. For fixing this bug first, I agree to change it to update the last only when compaction goes successfully.

WIZARD-CXY · 2018-03-16T02:20:41Z

I get why the test didn't pick up this finally, below code is from periodic_test.go

// simulate 5 hours worth of intervals.
	for i := 0; i < n/retentionHours*5; i++ {
		rg.Wait(1)
		fc.Advance(checkCompactInterval)
		// compaction doesn't happen til 2 hours elapses.
		if i < n {
			continue
		}
		// after 2 hours, compaction happens at every checkCompactInterval.
		a, err := compactable.Wait(1)
		if err != nil {
			t.Fatal(err)
		}
		expectedRevision := int64(i + 1 - n)
		if !reflect.DeepEqual(a[0].Params[0], &pb.CompactionRequest{Revision: expectedRevision}) {
			t.Errorf("compact request = %v, want %v", a[0].Params[0], &pb.CompactionRequest{Revision: expectedRevision})
		}
	}

because as @fanminshi's comment "// after 2 hours, compaction happens at every checkCompactInterval."
it is like "by design" 。。。。

WIZARD-CXY · 2018-03-16T08:39:58Z

changed according to the comment, but I don't know quite understand how this test works. so don't change it now, @gyuho can u give another test code?

xiang90 · 2018-03-16T18:52:30Z

@fanminshi can you take a final look at this one?

fanminshi · 2018-03-16T20:15:15Z

@xiang90 yes I am taking a look at this issue.

fanminshi · 2018-03-16T22:32:01Z

I did a more in-depth digging into what is going on with #8563.

Before change in #8563:

etcd only supports compaction retention at the hour level.
For instance, if I want to retain 1 hour worth history, I set etcd flag --auto-compaction-retention '1'.
Then etcd waits for an hour, then does the compaction at the rev that's an hour ago; it waits another hour and does the same. e.g

0Hr (rev = 1) -> 1Hrs (Compact(1), rev = 10 ) -> 2Hrs(Compact(10), rev = 20) -> ...

Hence, in this case the etcd always retain at least 1 hr worth of history
because it compacts its database with the revision recorded at previous hour.

However, what if I want to retain 5hrs worth of history where I set etcd flag --auto-compaction-retention '5'.
you would expect that etcd compacts every 5 hours as following:

0Hr (rev = 1) -> 5Hrs (Compact(1), rev = 50 ) -> 10Hrs(Compact(50), rev = 100) -> ...

However, that's not the case. etcd actually first waits 5hrs for the first compaction and then does compaction every hour afterward like this:

0Hr (rev = 1) -> 1hr (rev = 10) -> 2hr(rev = 20) -> 3hr(rev = 30) -> 4hr( rev = 40 )-> 5Hrs (Compact(1), rev = 50 ) -> 6Hrs(Compact(10), rev = 60) -> 7Hrs(Compact(20), rev = 70) ...

The above was the default behavior (please correct me if I am wrong).

After change in #8563:

The purpose of #8563 is to support the compaction retention at user defined level
such as 5s, 5m, 5hr, and so forth.

Suppose that the user specifies retention policy to be 5hr, should etcd first wait 5hr and then compact every hour afterward as shown in the default behavior?

More, if the user specifies retention policy to be 5s, should etcd first wait for 5s and compact every 1s or 5s afterward? The 5s behavior was not clear from the original implementation.

Hence, the #8563 decides to divide the user defined retention AutoCompactionRetention by
10 to compute a checkCompactInterval(e.g AutoCompactionRetention=10s -> checkCompactInterval=1s).
Use those, the current auto-compaction-retention behavior is that it first waits for AutoCompactionRetention, perform a compaction
immediately, and compacts every checkCompactInterval afterward.
e.g using AutoCompactionRetention=10s and checkCompactInterval=1s

0s (rev = 1) -> 1s(rev = 1) -> 2s(rev=2) ... -> 10s (Compact(1), rev = 10) -> 11s (Compact(1), rev = 11) -> 12s -> (Compact(2), rev = 12)...

Question to ask?
#8563 introduces a new behavior which might have seen as a bug.
Is the current behavior reasonable or not? if not what should it be.

fanminshi · 2018-03-16T22:58:48Z

@WIZARD-CXY
"// after 2 hours, compaction happens at every checkCompactInterval." it is like "by design" 。。。。
It was by design to have this behavior.

I see two options going forward with this.

Keep the current behavior as it is with a better documentation.
Change the auto retention policy to be that compact every AutoCompactionRetention defined by the user.

Then update the test to reflect the agreed behavior.

gyuho · 2018-03-16T23:03:42Z

5hrs for the first compaction and then does compaction every hour afterward like this:

I think we should reset compact interval on success, so that we only retain by the given retention number in hours. This is how v3.2 --auto-compaction-retention works (e.g. --auto-compaction-retention=10 compacts every 10-hour with latest revision). But in v3.3, --auto-compaction-retention=10h compacts at first 10-hour and compacts every hour afterwards. Think we should we it in v3.2 way. Otherwise, it's breaking change?

fanminshi · 2018-03-16T23:11:45Z

This is how v3.2 --auto-compaction-retention works (e.g. --auto-compaction-retention=10 compacts every 10-hour with latest revision)

@gyuho are you sure about this.
Read this commentCompaction happens hourly. from v3.2.17
https://github.com/coreos/etcd/blob/28c47bb2f8d4a0b60bd41dc5ff61016cff1cfb84/compactor/compactor.go#L45-L46

gyuho · 2018-03-16T23:15:01Z

ah, executeCompactionInterval is hour, hard-coded.

fanminshi · 2018-03-16T23:43:45Z

@gyuho yeah, we need to give more thought on the correct behavior before continuing with this pr.

xiang90 · 2018-03-17T00:36:18Z

@fanminshi

see #7868.

For hourly compaction, it makes sense to run more frequently to keep X hour history. For short compaction time, probably we can just compact every X interval.

WIZARD-CXY · 2018-03-17T03:30:13Z

@fanminshi I like the option2 "Change the auto retention policy to be that compact every AutoCompactionRetention defined by the user." It is straightforward and right match for my case.

fanminshi · 2018-03-19T15:10:10Z

@WIZARD-CXY the behavior was like option 2, but it was changed because of #7868.

For hourly compaction, it makes sense to run more frequently to keep X hour history. For short compaction time, probably we can just compact every X interval.
We can implement the behavior just like @xiang90 suggested.

yudai · 2018-03-19T18:44:13Z

Let me provide some context. I once modified the periodic compactor to run auto compaction hourly (#7868, #7875). Before #7875, when you set 10 hours, etcd can keep revisions for up to 20hours. This means you need to prepare your DB quota for 20 hours data. It's not a big problem for shorter duration, but if you set a larger compaction retention time, the overhead can be too large (especially, when the cap was limited to up to 8GB).

I once thought that we could define another flag for auto compaction interval, separated from auto compaction retention time or retention revisions. But I thought it was over-engineering at the moment. However, it could be worth considering, given we can see different needs for the auto compaction interval.

The 5 minute interval is used for the revision compactor as well. I therefore think we want to check if the interval is reasonable for the revision compactor as well.

For the default value for the periodic compactor. As @xiang90 suggested, I think that X when X <= 1 h, and 1h when X > 1 h would be reasonable. In this case, the maximum history is capped to retention time + 1h, which is stable enough for the DB size, and also not too frequent to run compactions.

yudai · 2018-03-20T22:09:06Z

Oops, sorry I had some misunderstanding on the current behavior.

The 5 minute interval is used for the revision compactor as well. I therefore think we want to check if the interval is reasonable for the revision compactor as well.

Please forget about this. The variable is not shared any longer from 3.3.

The current interval is X/10, so the over head is always X/10 (e.g. 1 hour for X=10, 2.4 hours for X=24). I think this is reasonable and we don't need to change the upper bound.

As already suggested, I agree that just setting the minimum interval is enough to prevent too aggressive auto compaction.

fanminshi · 2018-03-20T22:56:15Z

I think that X when X <= 1 h, and 1h when X > 1 h would be reasonable.
Let's just do this.

gyuho added backport/v3.3 Release-Backport/v3.2 and removed Release-Backport/v3.2 labels Mar 15, 2018

fanminshi reviewed Mar 15, 2018

View reviewed changes

WIZARD-CXY mentioned this pull request Mar 16, 2018

*: support auto-compaction with finer granularity #8563

Merged

fix bug in auto-compact

b8769eb

WIZARD-CXY force-pushed the fixbug branch from 6f79f13 to b8769eb Compare March 16, 2018 08:36

yudai mentioned this pull request Mar 19, 2018

default value of auto-compaction in 3.2.x #9456

Closed

gyuho mentioned this pull request Mar 20, 2018

*: document auto-compaction logic #9468

Merged

gyuho mentioned this pull request Mar 21, 2018

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

Closed

gyuho closed this Mar 21, 2018

gyuho removed backport/v3.3 Release-Note labels Mar 22, 2018

gyuho mentioned this pull request Mar 23, 2018

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bug in auto-compact #9443

fix bug in auto-compact #9443

WIZARD-CXY commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

xiang90 commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

gyuho commented Mar 15, 2018

fanminshi Mar 15, 2018

gyuho Mar 15, 2018

fanminshi Mar 15, 2018

gyuho Mar 15, 2018

WIZARD-CXY Mar 16, 2018

xiang90 Mar 16, 2018

WIZARD-CXY Mar 16, 2018

xiang90 Mar 16, 2018

WIZARD-CXY Mar 16, 2018

WIZARD-CXY Mar 16, 2018

WIZARD-CXY commented Mar 16, 2018

WIZARD-CXY commented Mar 16, 2018

xiang90 commented Mar 16, 2018

fanminshi commented Mar 16, 2018 •

edited

Loading

fanminshi commented Mar 16, 2018 •

edited

Loading

fanminshi commented Mar 16, 2018

gyuho commented Mar 16, 2018 •

edited

Loading

fanminshi commented Mar 16, 2018 •

edited

Loading

gyuho commented Mar 16, 2018

fanminshi commented Mar 16, 2018

xiang90 commented Mar 17, 2018

WIZARD-CXY commented Mar 17, 2018

fanminshi commented Mar 19, 2018

yudai commented Mar 19, 2018 •

edited

Loading

yudai commented Mar 20, 2018 •

edited

Loading

fanminshi commented Mar 20, 2018

fix bug in auto-compact #9443

fix bug in auto-compact #9443

Conversation

WIZARD-CXY commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

xiang90 commented Mar 15, 2018

WIZARD-CXY commented Mar 15, 2018

gyuho commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WIZARD-CXY commented Mar 16, 2018

WIZARD-CXY commented Mar 16, 2018

xiang90 commented Mar 16, 2018

fanminshi commented Mar 16, 2018 • edited Loading

fanminshi commented Mar 16, 2018 • edited Loading

fanminshi commented Mar 16, 2018

gyuho commented Mar 16, 2018 • edited Loading

fanminshi commented Mar 16, 2018 • edited Loading

gyuho commented Mar 16, 2018

fanminshi commented Mar 16, 2018

xiang90 commented Mar 17, 2018

WIZARD-CXY commented Mar 17, 2018

fanminshi commented Mar 19, 2018

yudai commented Mar 19, 2018 • edited Loading

yudai commented Mar 20, 2018 • edited Loading

fanminshi commented Mar 20, 2018

fanminshi commented Mar 16, 2018 •

edited

Loading

fanminshi commented Mar 16, 2018 •

edited

Loading

gyuho commented Mar 16, 2018 •

edited

Loading

fanminshi commented Mar 16, 2018 •

edited

Loading

yudai commented Mar 19, 2018 •

edited

Loading

yudai commented Mar 20, 2018 •

edited

Loading