*: support auto-compaction with finer granularity #8563

fanminshi · 2017-09-14T23:15:03Z

fanminshi · 2017-09-14T23:17:51Z

Some manual test result:

test 5s interval:

$ bin/etcd --auto-compaction-retention 5s
2017-09-14 16:11:32.726296 N | compactor: Starting auto-compaction at revision 1 (retention: 5s )
2017-09-14 16:11:32.729183 I | mvcc: store.index: compact 1
2017-09-14 16:11:32.729237 N | compactor: Finished auto-compaction at revision 1
2017-09-14 16:11:32.729302 I | mvcc: finished scheduled compaction at 1 (took 41.163µs)
2017-09-14 16:11:37.726362 N | compactor: Starting auto-compaction at revision 1 (retention: 5s )
2017-09-14 16:11:37.726519 N | compactor: Finished auto-compaction at revision 1
2017-09-14 16:11:42.726478 N | compactor: Starting auto-compaction at revision 1 (retention: 5s )
2017-09-14 16:11:42.726642 N | compactor: Finished auto-compaction at revision 1

test no compaction:

$ bin/etcd --auto-compaction-retention 0
2017-09-14 16:12:08.019460 N | embed: serving insecure client requests on 127.0.0.1:2379, this is strongly discouraged!

test auto-compaction doesn't compact at every 1 time.Duration(1):

$bin/etcd --auto-compaction-retention 1
2017-09-14 16:12:30.743382 N | embed: serving insecure client requests on 127.0.0.1:2379, this is strongly discouraged!

fanminshi · 2017-09-14T23:18:48Z

if the approach seems fine, I'll refactor the tests for compactor as it is currently tightly couple with compactor's implementation.

gyuho · 2017-09-14T23:35:52Z

compactor/periodic.go

+				if err == nil || err == mvcc.ErrCompacted {
+					plog.Noticef("Finished auto-compaction at revision %d", rev)
+				} else {
+					plog.Noticef("Failed auto-compaction at revision %d (%v)", err, rev)


, rev, err?

heyitsanthony · 2017-09-15T07:18:45Z

compactor/periodic.go

-				continue
-			}
-
-			rev, remaining := t.getRev(t.periodInHour)


it's not necessary to remove this to support the feature and removing it completely changes the behavior of the periodic compactor

xiang90 · 2017-09-21T20:13:58Z

compactor/periodic.go

-				plog.Noticef("Failed auto-compaction at revision %d (%v)", err, rev)
-				plog.Noticef("Retry after %v", checkCompactionInterval)
+				t.mu.RUnlock()
+				rev := t.rg.Rev()


this is wrong. we should keep the previous logic to compact the revision of the past last x duration.

fanminshi · 2017-09-26T18:18:52Z

@xiang90 I changed the compactor logic with following:

get store rev now.
wait for retention period to pass.
compact at rev from step 1.
go to step 1.

xiang90 · 2017-09-26T18:21:11Z

why do we change the previous logic? i feel all we need to do there is to change the wait time from a fix x hours to some user given timeout, no?

fanminshi · 2017-09-26T18:28:06Z

@xiang90 the previous logic has a checkCompactionInterval that defaults to 5 mins. So the loop runs every 5 minutes to check if it should compact. It works for any wait time > 5 mins but not < 5 mins. therefore, I get rid of it. Having a looping checking every 5 minutes seems unnecessary. All the compactor really care is the rev at time.Now() - retentionPeriod. So I don't see the need to keep tracks revs every 5 minutes and then get the right rev through some math when needs to compact.

xiang90 · 2017-09-26T18:41:05Z

@fanminshi

There are a few issues with your approach, which the old approach addressed.

short retry time when error occurs (5 minutes instead of 1hr)
not updating compaction rev when error occurs

I would suggest you understand how the previous code handles failures before simplifying it. The rev recording part can be simplified though.

fanminshi · 2017-09-26T19:36:41Z

@xiang90 i see. let me rethink about the error handling part.

fanminshi · 2017-09-26T21:10:54Z

@xiang90 i add fast retry behavior into the code.

xiang90 · 2017-09-26T21:14:06Z

this is getting more complicated. i am wondering why not keep the exact previous logic and change the checkInterval to min(user-specified-timeout/10, 1*second) or something.

fanminshi · 2017-09-26T21:27:15Z

@xiang90 checking every checkCompactionInterval to see if I should compact seems a bit heavy to me. I think there can be a more efficient way. Anyways, I can keep the original approach by changing checkCompactionInterval to a appropriate value.

xiang90 · 2017-09-26T21:30:09Z

checking every checkCompactionInterval to see if I should compact seems a bit heavy to me. I think there can be a more efficient way.

99% people will compact > 1 minute interval. that is max 0.16 read request / second, which is almost nothing.

fanminshi · 2017-09-26T23:27:59Z

@xiang90 redone the pr with previous approach. PTAL

edit: Thing to notice. the original code compacts every hour after T > retentionPeriod. That must be change for <1 hour compaction retention. So I change the compaction frequency to every checkCompactionInterval when T > retentionPeriod.

xiang90 · 2017-09-27T00:09:20Z

compactor/periodic.go

 	}
 }

+// N divides Periodic.period in into checkCompactionInterval duration
+var N = 10


why not a const? why make this public?

probably name this to something more descriptive too.

yeah, i'll come up with a better name.

xiang90 · 2017-09-27T00:09:57Z

compactor/periodic.go

@@ -66,31 +70,27 @@ func (t *Periodic) Run() {
 			case <-t.ctx.Done():
 				return
 			case <-clock.After(checkCompactionInterval):
-				t.mu.Lock()
+				t.mu.RLock()


why change to use rlock here?

i noticed that we are just reading t.paused. so I used rlock.

the critical area is super tight and not accessed frequently. it probably not worths changing it.

if you want to make it more efficient, do it in another PR. Let us keep one PR for one thing.

alright then

xiang90 · 2017-09-27T00:10:56Z

compactor/periodic.go

@@ -113,8 +113,8 @@ func (t *Periodic) Resume() {
 	t.paused = false
 }

-func (t *Periodic) getRev(h int) (int64, []int64) {
-	i := len(t.revs) - int(time.Duration(h)*time.Hour/checkCompactionInterval)
+func (t *Periodic) getRev() (int64, []int64) {


pass in n? probably we do not need the global defined const n.

the n can be used in testing. that's why I had a const n.

ah. ok. makes sense.

xiang90 · 2017-09-27T00:11:57Z

left some comments about minor issues. defer @gyuho or @jpbetz for a final look.

gyuho · 2017-09-27T18:12:36Z

@fanminshi Fix Travis CI failures?

jpbetz

Looks good in general. Thanks for handling the backward compatibility of the flags carefully.

Just a couple comments on code structure and documentation.

jpbetz · 2017-09-27T21:10:48Z

etcdmain/config.go

-	fs.IntVar(&cfg.AutoCompactionRetention, "auto-compaction-retention", 0, "Auto compaction retention for mvcc key value store. 0 means disable auto compaction.")
-	fs.StringVar(&cfg.AutoCompactionMode, "auto-compaction-mode", "periodic", "Interpret 'auto-compaction-retention' as hours when 'periodic', as revision numbers when 'revision'.")
+	fs.StringVar(&cfg.AutoCompactionRetention, "auto-compaction-retention", "0", "Auto compaction retention for mvcc key value store. 0 means disable auto compaction.")
+	fs.StringVar(&cfg.AutoCompactionMode, "auto-compaction-mode", "periodic", "'periodic' means hours if an integer or a duration string otherwise, 'revision' means revision numbers to retain by auto compaction")


I found the Interpret 'auto-compaction-retention' as.. preamble helpful. It directly relates the two flags. Keep it? Maybe:

Interpret 'auto-compaction-retention' one of: periodic|revision. 'periodic' for duration based retention, defaulting to hours if no time unit is provided (e.g. '5m'). 'revision' for revision number based retention. ?

your suggestion seems clearer. I'll change to that.

jpbetz · 2017-09-27T21:38:22Z

compactor/periodic.go

@@ -73,24 +77,20 @@ func (t *Periodic) Run() {
 					continue
 				}
 			}
-
-			if clock.Now().Sub(last) < executeCompactionInterval {
+			if clock.Now().Sub(last) < t.period {


Can we simplify the timer logic a bit here? I found the logic a bit difficult to follow with the case <-clock.After(checkCompactionInterval): select case and the periodDivisor combined together.

Could we instead do something like

select { ... case <-clock.After(t.period - clock.Now().Sub(last)): ...

and then eliminate this if clock.Now().Sub(last) < t.period { check and the periodDivisor?

(let me know if I'm missing an important subtlety to how the timers are being use here...)

@jpbetz I am trying to minimize any change. we can refactor the code in a future pr if you want.

Sounds good.

jpbetz · 2017-09-27T21:41:26Z

compactor/periodic.go

@@ -73,24 +77,20 @@ func (t *Periodic) Run() {
 					continue
 				}
 			}
-
-			if clock.Now().Sub(last) < executeCompactionInterval {


While we're improving this function. Can move the remaining code in the for loop into the case <-clock.After for readability? It's only ever run as part of that case...

can we refactor the code in a future pr?

fanminshi · 2017-09-28T21:06:53Z

made the auto-compaction-mode more clear.

xiang90 · 2017-09-28T21:08:57Z

lgtm. defer to @gyuho

@jpbetz let's improve the code in the next pr.

xiang90 · 2017-09-28T21:09:26Z

compactor/compactor.go

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

 const (
-	checkCompactionInterval   = 5 * time.Minute
-	executeCompactionInterval = time.Hour
+	checkCompactionInterval = 5 * time.Minute


this const can also be removed.

checkCompactionInterval is used Revision compactor.
https://github.com/coreos/etcd/blob/master/compactor/revision.go#L64
I don't think it should be removed.

gyuho · 2017-09-28T23:24:54Z

compactor/periodic.go

 			if rev < 0 {
 				continue
 			}
-
-			plog.Noticef("Starting auto-compaction at revision %d (retention: %d hours)", rev, t.periodInHour)
+			plog.Noticef("Starting auto-compaction at revision %d (retention: %v )", rev, t.period)


s/(retention: %v )/(retention: %v)/

gyuho · 2017-09-29T00:16:21Z

etcdmain/help.go

@@ -99,7 +99,7 @@ clustering flags:
 	--auto-compaction-retention '0'
 		auto compaction retention length. 0 means disable auto compaction.
 	--auto-compaction-mode 'periodic'
-		'periodic' means hours, 'revision' means revision numbers to retain by auto compaction
+		Interpret 'auto-compaction-retention' one of: periodic|revision. 'periodic' for duration based retention, defaulting to hours if no time unit is provided (e.g. '5m'). 'revision' for revision number based retention.


interpret to make casing consistent?

fanminshi · 2017-09-29T00:33:02Z

embed/etcd.go

+		h                       int
+	)
+	// AutoCompactionRetention defaults to "0" if not set.
+	if len(cfg.AutoCompactionRetention) == 0 {


added a check see if AutoCompactionRetention is not set.

fanminshi · 2017-09-29T18:14:42Z

merge when green.

WIZARD-CXY · 2018-03-15T05:46:59Z

compactor/periodic.go

 			_, err := t.c.Compact(t.ctx, &pb.CompactionRequest{Revision: rev})
 			if err == nil || err == mvcc.ErrCompacted {
 				t.revs = remaining
-				last = clock.Now()


why we delete this line?

good a catch. I don't think this is meant to be deleted.

yeah, it actually causes a bug as #9443

Actually, I dig into on why last = clock.Now() is deleted again. It is explained:

I change the compaction frequency to every checkCompactionInterval when T > retentionPeriod.
#8563 (comment)

I think that's cause.

fanminshi added the WIP label Sep 14, 2017

gyuho reviewed Sep 14, 2017

View reviewed changes

gyuho added the kind/release-note label Sep 14, 2017

fanminshi force-pushed the make_auto_compaction_granular branch from b0e83d6 to ce4d657 Compare September 14, 2017 23:44

heyitsanthony reviewed Sep 15, 2017

View reviewed changes

xiang90 reviewed Sep 21, 2017

View reviewed changes

fanminshi force-pushed the make_auto_compaction_granular branch from ce4d657 to a3364ce Compare September 26, 2017 18:10

fanminshi force-pushed the make_auto_compaction_granular branch from a3364ce to 0d3bea6 Compare September 26, 2017 21:10

fanminshi force-pushed the make_auto_compaction_granular branch 2 times, most recently from ff350b5 to da7f6d7 Compare September 26, 2017 23:27

fanminshi removed the WIP label Sep 26, 2017

fanminshi force-pushed the make_auto_compaction_granular branch from da7f6d7 to e88267f Compare September 26, 2017 23:42

xiang90 reviewed Sep 27, 2017

View reviewed changes

fanminshi force-pushed the make_auto_compaction_granular branch from e88267f to e8b35de Compare September 27, 2017 00:21

jpbetz reviewed Sep 27, 2017

View reviewed changes

jpbetz mentioned this pull request Sep 27, 2017

Give the “Compactor” work to etcd kubernetes/kubernetes#46705

Closed

fanminshi force-pushed the make_auto_compaction_granular branch from e8b35de to 8cc9995 Compare September 28, 2017 21:05

xiang90 reviewed Sep 28, 2017

View reviewed changes

fanminshi force-pushed the make_auto_compaction_granular branch from 8cc9995 to c38b923 Compare September 28, 2017 21:19

gyuho reviewed Sep 28, 2017

View reviewed changes

fanminshi force-pushed the make_auto_compaction_granular branch from c38b923 to b4297d4 Compare September 29, 2017 00:10

gyuho reviewed Sep 29, 2017

View reviewed changes

fanminshi added 2 commits September 28, 2017 17:22

*: modify etcd flags to support finner compaction retention

733de98

compactor: support finer retention period in compactor.go

2532594

fanminshi force-pushed the make_auto_compaction_granular branch from b4297d4 to 2532594 Compare September 29, 2017 00:23

etcdmain: check for empty AutoCompactionRetention

0e1993f

fanminshi commented Sep 29, 2017

View reviewed changes

fanminshi merged commit bcef78c into etcd-io:master Sep 29, 2017

WIZARD-CXY reviewed Mar 15, 2018

View reviewed changes

WIZARD-CXY mentioned this pull request Mar 15, 2018

fix bug in auto-compact #9443

Closed

*: support auto-compaction with finer granularity #8563

*: support auto-compaction with finer granularity #8563

Conversation

fanminshi commented Sep 14, 2017

fanminshi commented Sep 14, 2017

fanminshi commented Sep 14, 2017

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 commented Sep 26, 2017

xiang90 commented Sep 26, 2017

fanminshi commented Sep 26, 2017 • edited Loading

xiang90 commented Sep 26, 2017

fanminshi commented Sep 26, 2017

fanminshi commented Sep 26, 2017

xiang90 commented Sep 26, 2017

fanminshi commented Sep 26, 2017

xiang90 commented Sep 26, 2017

fanminshi commented Sep 26, 2017 • 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

Choose a reason for hiding this comment

xiang90 commented Sep 27, 2017

gyuho commented Sep 27, 2017

jpbetz left a 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

fanminshi commented Sep 28, 2017

xiang90 commented Sep 28, 2017

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

fanminshi commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

fanminshi commented Sep 26, 2017 •

edited

Loading

fanminshi commented Sep 26, 2017 •

edited

Loading

fanminshi Mar 16, 2018 •

edited

Loading