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

gyuho · 2018-03-21T18:57:43Z

If given compaction period x is <1-hour, compact every x duration, with x retention window (e.g. --auto-compaction-mode 'periodic' --auto-compaction-retention='10m', then compact every 10-minute).

If given compaction period x is >1-hour, compact every 1-hour, with x retention window (e.g. --auto-compaction-mode 'periodic' --auto-compaction-retention='72h', then compact every 1-hour). Retry in x/10 duration on compaction failure.

gyuho · 2018-03-21T18:58:43Z

/cc @yudai @fanminshi

fanminshi · 2018-03-21T19:59:05Z

compactor/periodic.go

 			} else {
 				plog.Noticef("Failed auto-compaction at revision %d (%v)", rev, err)
-				plog.Noticef("Retry after %v", checkCompactInterval)
+				plog.Noticef("Retry after %v", interval)


This changes the behavior of retry when error. Previously, we retry quickly at 1/10 the of the retention period and before that we retry every 5min. With this change, with retention period > 1Hr, we will retry again in 1Hr.

@yudai We are still discussing better ways to implement retry logic on compact failures. Any thoughts?

Retrying in 1 hour sounds a bit longer to me. 5 minutes or retension/10 would be more reasonable.

By the way, If I understand correctly, with this code, we don't slide t.revs in the error case, and t.revs gets a new value in the beginning of the next loop iteration (line 65) anyway, meaning the len(t.revs) increases when an error happens. Eventually, the retention time unexpectedly gets another 1 hour for each error. I think we should slide t.revs in the error case as well, if we keep this retry logic. Another idea is make line 82 like rev := t.revs[len(t.revs) - interval].

The main point is that t.revs = append(t.revs, t.rg.Rev()) at line 65 is expected to be executed every interval. To keep this promise, we cannot add another timed task for retry in the loop.
We might use another gorotuine for retry to keep the main loop running, but I feel like it's an error prone way.
Maybe a better idea is that using a divided interval (by 10 or something, e.g. 6 minutes interval for 1hour retention period). And at line 82, we can do rev := t.revs[len(t.revs) - interval*10]? (we also need to modify line 89). To avoid running compaction every retention/10, we would need a counter as well (we reset it when compaction succeeds).
In this case, we can retry in 1/10 retention time.

I'm still not sure if my idea is feasible, I'll try implement it and see how it looks like.

@yudai Sure. We will try implementing simplest retry logic first (for 3.3 backport). And iterate from there to improve. Thanks.

The idea would look like this? However, I realized that if the retention time is small, the subInterval (interval/10) will be very very small. So this might not be a good idea to go.

https://gist.github.com/yudai/d976432a71e140598c08c91ae8569435

Using two goroutine looks simpler in a sense and maybe most efficient. The retry interval is interval/10.

https://gist.github.com/yudai/da0cf189c8c445973615671b33765c7d

@gyuho these two implementations are what I came up with. I personally like the way with two goroutines, but it's a big modification, so not sure if it's acceptable.

The retry interval is interval/10

I think this is a good idea since it won't be breaking change for 3.3.

And for two goroutines, let's do it once we backport this fix?
You can create a separate PR once we address this PR.
I will rework on this PR.

Thanks!

fanminshi · 2018-03-21T20:09:30Z

compactor/compactor.go

@@ -29,8 +29,6 @@ var (
 )

 const (
-	checkCompactionInterval = 5 * time.Minute


maybe move this to the revision compactor since it is the only one who is using it?

fanminshi · 2018-03-21T20:10:34Z

compactor/revision.go

@@ -17,6 +17,7 @@ package compactor
 import (


no need to refactor revision compactor code? it is work as expected? we can refactor this in the future pr.
let's make this pr at a minimum?

codecov-io · 2018-03-21T20:34:58Z

Codecov Report

❗ No coverage uploaded for pull request base (master@31de834). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #9474   +/-   ##
=========================================
  Coverage          ?   72.59%           
=========================================
  Files             ?      364           
  Lines             ?    30885           
  Branches          ?        0           
=========================================
  Hits              ?    22420           
  Misses            ?     6842           
  Partials          ?     1623

Impacted Files	Coverage Δ
compactor/periodic.go	`86.53% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31de834...9adb346. Read the comment docs.

yudai · 2018-03-21T20:47:56Z

compactor/periodic.go

-	checkCompactInterval := t.period / time.Duration(periodDivisor)
+	interval := t.getInterval()
+
+	last := t.clock.Now()


Renaming to something like initialWait? I think last is confusing.

yudai · 2018-03-22T18:24:12Z

What's interesting here is;

if you set 60 minutes, compaction happens every 6 minutes
if you set 59 minutes, compaction happens every 59 minutes

I guess this jump maybe confusing and we will get a bug report about this jump in the future.

gyuho · 2018-03-22T18:29:48Z

@yudai Yeah, this would break old behavior either way.

fanminshi · 2018-03-22T18:40:28Z

compactor/periodic.go

+func (t *Periodic) getInterval() time.Duration {
+	itv := t.period
+	if itv > time.Hour {
+		itv /= 10


why is this changed? I thought we are going to do x is >1-hour -> compact every hr after initial wait?

This is to keep the old behavior. But at this point, we break old behavior anyways.

if you set 60 minutes, compaction happens every 6 minutes if you set 59 minutes, compaction happens every 59 minutes

The above behavior can be avoided if we do x is >1-hour -> compact every hr after initial wait??

As you pointed out #9474 (comment), that would break old behavior. If we don't backport, I am fine with this too.

The original code introduced a concept of checkInterval. This interval just to check if I should do compaction or not? it can be quick we had it set as 5 mins . So If a compact error happens, we will just wait another 5 mins to check again. We might want to keep that check interval. The interval in this pr is the compact interval which is different from the check interval.

This interval just to check if I should do compaction or not? it can be quick we had it set as 5 mins . So If a compact error happens, we will just wait another 5 mins to check again.

5-min interval is for revision compactor. So, v3.3.2 still retries after 1/10 of given compaction period. https://github.com/coreos/etcd/blob/v3.3.2/compactor/periodic.go#L64

Discussed in person. @fanminshi meant 5-min in v3.2 :)

yudai · 2018-03-22T18:48:47Z

We can define new rules like below if we don't like the jump, but looks a bit complicated.

compact every x/10 duration, with x retention window
when x/10 is smaller than Y-minute, interval will be smaller value of Y-minutes or x

(you can put a threshold number to Y, like 6 minutes)

gyuho · 2018-03-22T18:53:10Z

@fanminshi @yudai Yeah, I think it's better to have minimum interval.

While keeping the old behavior of 1/10 of given period, we just introduce hard-coded limit of 10-minute(???). Then, 1-hour compaction runs every 10-minute. 20-minute compaction runs every 10-minute, rather than 2-minute.

The main motivation is prevent aggressive auto-compaction as in #9443

etcd --auto-compaction-retention 5s

What do you think?

yudai · 2018-03-22T18:56:56Z

@gyuho A simple lower-bound limit makes an issue when you set an under-boundary value to the period.

For example, when the limit is 10-minute, if you put 5m to the period, we probably wan to run compaction every 5-minute, not every 10-minute.

when x/10 is smaller than Y-minute, interval will be smaller value of Y-minutes or x

comes from this point.

Let the threshold 6 minutes,

X > 60m, interval is X/10
6 <= X <= 60 , interval is always 6 minutes
X < 6, interval is X

There is no jumping, but a bit tricky.

fanminshi · 2018-03-22T19:03:28Z

I think the idea "when x > 1hr, compact every hr after x, and When x < 1h, compact every x" is simpler reason about.

if x = 1hr, compacts every 1hr.
If x = 2hr, waits 2h and compacts every 1hr.
If x = 59 min, waits 59min and compacts every 59 min.
if x = 5s, waits 5s and compacts 5s.

What's the issue with the above behavior?

cc/ @gyuho @yudai

EDIT: add failure path

If a compaction failed:
we should retry at 1/10 of the compaction intevals.

if x = 1hr, compacts every 1hr. if compaction has failed, retry again in 6 mins.
if x = 2hr, waits 2h and compacts every 1hr. if compaction has failed, retry again in 6 mins.
If x = 59 min, waits 59min and compacts every 59 min. if compaction has failed, retry again in 5.9 mins
if x = 5s, waits 5s and compacts 5s. if compaction has failed, retry again in 500ms.

yudai · 2018-03-22T19:09:34Z

@fanminshi I think "when x > 1hr, compact every hr after x, and When x < 1h, compact every x" is pretty fair and easy to understand. If we can choose it, I think it's the best way to go.

The only point that we need to consider is that this behavior is not compatible with what we have with v3.3 (right?). So if we backport this PR to v3.3, it's a breaking change for v3.3. (If we deem that the current v3.3 behavior is a bug, we can backport this PR).

I think @gyuho is trying to keep the backward compatiblity with v3.3 in this PR.

yudai · 2018-03-22T19:12:24Z

Given that we are not going to break v3.3 behavior in this PR, 'X/10' interval is the baseline. In that sense, the only thing what we can do here is that removing too aggressive auto compaction done by X<1h period.

gyuho · 2018-03-22T19:16:13Z

What's the issue with the above behavior?

It breaks old behavior that sets interval as 1/10 of given period. So, let's do this for v3.4 (current master branch), given that this PR only wants to address #9443.

@yudai's logic in #9474 (comment) sounds reasonable, while keeping the old behavior at its best.

fanminshi · 2018-03-22T19:20:27Z

If we deem that the current v3.3 behavior is a bug, we can backport this PR
I think that's what user thinks as raised by @WIZARD-CXY. I think we should backport this pr to v3.3. Let's fix the pain now before we changed it again in 3.4 which might cause more confusion later on.

gyuho · 2018-03-22T19:25:53Z

Yeah, I will do as we first planned. Might be too confusing for users.
I will instead explain these changes more explicitly in our docs.

gyuho · 2018-03-22T19:34:33Z

Just pushed with our first plan. Will document this separately.

yudai · 2018-03-22T19:56:40Z

compactor/periodic.go

 				plog.Noticef("Finished auto-compaction at revision %d", rev)
 			} else {
+				// retry fast
+				interval = t.getInterval() / 10


I think in retry iterations, we don't want to update t.revs.
Maybe something like below is required at line 70.

if interval == t.getInterval() { t.revs = append(t.revs, t.rg.Rev()) }

@yudai Good catch. Just fixed. PTAL. Thanks.

Ah sorry, I have another corner case.

Now t.revs is not updated while retry is ongoing. But if compaction keeps failing for a while, it will be outdated. So ideally, we need to update t.revs and shift the window once every 10 times when retry is ongoing to keep t.revs[0] fresh.

One more thing. I think it's just about definition of "interval". If retry fails 9 times, next compaction with a new rev happens at (9/10 + 1) hour. So the effective interval and the DB overhead can vary between 1 hour through 1.9 hours. But if we define the interval as "the duration from the last SUCCESSFUL compaction", it's just fine.

I guess without using two goroutines, these are hard to implement simpler. You may want to put these corner cases aside in this PR.

gyuho · 2018-03-22T22:02:34Z

@yudai Good point.

I would see "interval" more of fetch interval and retry interval. And compaction period is more of retention window. We want to maintain this sliding window up-to-date, no matter how many times compaction fails or succeeds. To address this, for every fetch interval, it always updates revs, so that it can keep first element up-to-date. And as you suggested, followed by a separate for-loop with retry interval in case compaction fails.

gyuho · 2018-03-22T22:05:44Z

compactor/periodic_test.go

 }

-func TestPeriodicPause(t *testing.T) {
+func TestPeriodicPauseHourly(t *testing.T) {


This shows what would happen when compactor fails for 30-hours.

yudai · 2018-03-22T22:22:25Z

compactor/periodic.go

+				p := t.paused
+				t.mu.RUnlock()
+				if p {
+					break


this break won't stop the loop, because it's captured by the select.

you can move the pause check block to the top of the loop and maybe merge another one at line 90 as well.

oh right. good catch. fixed.

yudai · 2018-03-22T22:23:49Z

compactor/periodic.go

+// divide by 10 to retry faster
+// e.g. given period 2-hour, retry in 12-min rather than 1-hour (compaction period)
+func (t *Periodic) getRetryInterval() time.Duration {
+	return t.period / retryDivisor


I guess this should be interval/retryDivisor for cases t.period > 1h

fixed. thanks @yudai !

yudai · 2018-03-22T22:39:52Z

compactor/periodic.go

+	if itv > time.Hour {
+		itv /= retryDivisor
+	}
+	return itv


When t.period is 10minutes for example, retryInterval will be 10 minutes. This is fine.
But retryDivisor is still 10 in this case, so Run() tries 10 times with 10 minute wait, meaning 100 minutes in total.
We may need to return retry divisor as well?

Right. Just fixed. Think we've covered all corner cases now? :)
I will take another look with @fanminshi tomorrow.

Thanks @yudai

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

yudai · 2018-03-22T22:50:39Z

We want to maintain this sliding window up-to-date, no matter how many times compaction fails or succeeds.

I think this is impossible ore pretty hard to write if we don't have two goroutins.

t.revs is updated right after a compaction succeeds in a happy path. Time gap between the last item and the 2nd from the last item in the slice is interval. This is what we want.
However, if retry happens, the gap will be up to 1.9 * interval with the current code.

gyuho · 2018-03-22T23:25:09Z

@yudai Yeah, I have another related corner case. I will rework on that.

yudai · 2018-03-22T23:37:05Z

If you don't mind, I can also work on this PR with two gorouines. Please just ping me.

gyuho · 2018-03-22T23:38:30Z

@yudai Sure, that would be better!

gyuho added the backport/v3.3 label Mar 21, 2018

fanminshi reviewed Mar 21, 2018

View reviewed changes

gyuho added the WIP - DO NOT MERGE label Mar 21, 2018

yudai reviewed Mar 21, 2018

View reviewed changes

gyuho mentioned this pull request Mar 22, 2018

compactor: clean up #9476

Merged

gyuho force-pushed the compactor branch from 4e71506 to e36779f Compare March 22, 2018 02:53

gyuho removed the WIP - DO NOT MERGE label Mar 22, 2018

gyuho changed the title ~~compactor: clean up, adjust retention window in periodic compaction~~ compactor: adjust interval for period <1-hour Mar 22, 2018

gyuho force-pushed the compactor branch 4 times, most recently from 086f74c to 9adb346 Compare March 22, 2018 04:13

fanminshi reviewed Mar 22, 2018

View reviewed changes

gyuho added the WIP - DO NOT MERGE label Mar 22, 2018

gyuho force-pushed the compactor branch from 9adb346 to bbf1555 Compare March 22, 2018 19:33

gyuho removed the WIP - DO NOT MERGE label Mar 22, 2018

yudai reviewed Mar 22, 2018

View reviewed changes

gyuho force-pushed the compactor branch 2 times, most recently from ad7f504 to f0bec73 Compare March 22, 2018 21:59

gyuho force-pushed the compactor branch from f0bec73 to c29f405 Compare March 22, 2018 22:04

gyuho commented Mar 22, 2018

View reviewed changes

yudai reviewed Mar 22, 2018

View reviewed changes

gyuho force-pushed the compactor branch from c29f405 to b4d9703 Compare March 22, 2018 22:29

yudai reviewed Mar 22, 2018

View reviewed changes

compactor: adjust interval for period <1-hour

9a6097b

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

gyuho force-pushed the compactor branch from b4d9703 to 9a6097b Compare March 22, 2018 22:45

gyuho added the WIP - DO NOT MERGE label Mar 22, 2018

yudai mentioned this pull request Mar 23, 2018

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

Closed

gyuho closed this Mar 23, 2018

gyuho removed backport/v3.3 WIP - DO NOT MERGE labels Mar 23, 2018

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

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

Conversation

gyuho commented Mar 21, 2018 • edited Loading

gyuho commented Mar 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yudai Mar 21, 2018 • edited Loading

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

codecov-io commented Mar 21, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

yudai commented Mar 22, 2018

gyuho commented Mar 22, 2018

fanminshi Mar 22, 2018 • edited Loading

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

fanminshi Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yudai commented Mar 22, 2018

gyuho commented Mar 22, 2018 • edited Loading

yudai commented Mar 22, 2018 • edited Loading

fanminshi commented Mar 22, 2018 • edited Loading

yudai commented Mar 22, 2018

yudai commented Mar 22, 2018 • edited Loading

gyuho commented Mar 22, 2018

fanminshi commented Mar 22, 2018

gyuho commented Mar 22, 2018

gyuho commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yudai Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

gyuho commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yudai Mar 22, 2018 • edited Loading

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

yudai commented Mar 22, 2018

gyuho commented Mar 22, 2018

yudai commented Mar 22, 2018

gyuho commented Mar 22, 2018

gyuho commented Mar 21, 2018 •

edited

Loading

yudai Mar 21, 2018 •

edited

Loading

codecov-io commented Mar 21, 2018 •

edited

Loading

fanminshi Mar 22, 2018 •

edited

Loading

fanminshi Mar 22, 2018 •

edited

Loading

gyuho commented Mar 22, 2018 •

edited

Loading

yudai commented Mar 22, 2018 •

edited

Loading

fanminshi commented Mar 22, 2018 •

edited

Loading

yudai commented Mar 22, 2018 •

edited

Loading

gyuho commented Mar 22, 2018 •

edited

Loading

yudai Mar 22, 2018 •

edited

Loading

gyuho commented Mar 22, 2018 •

edited

Loading

yudai Mar 22, 2018 •

edited

Loading