Compactor: Add Revisional compactor #8123

yudai · 2017-06-17T00:32:33Z

Hello,

For #8098, adding a new compactor named Revisional, which runs compaction every 5 minutes.
To switch the compactor from the default Periodic, I added a new option --auto-compaction-mode as well.

Some Notes:

Should Periodic run compaction every 5 minutes as well for consistency? (currently it runs hourly)
The new option might be little confusing. Instead of numbers, we might want to use human readable labels such as periodic and revisional.

heyitsanthony

general approach looks OK; some minor nits. It'd be nice to consolidate duplicated the revisional/periodic code, but I don't see an obvious way to do it at the moment

heyitsanthony · 2017-06-19T18:21:34Z

etcdmain/config.go

@@ -199,7 +199,8 @@ func newConfig() *config {
 	// version
 	fs.BoolVar(&cfg.printVersion, "version", false, "Print the version and exit.")

-	fs.IntVar(&cfg.AutoCompactionRetention, "auto-compaction-retention", 0, "Auto compaction retention for mvcc key value store in hour. 0 means disable auto compaction.")
+	fs.IntVar(&cfg.AutoCompactionRetention, "auto-compaction-retention", 0, "Auto compaction retention for mvcc key value store. 0 means disable auto compaction.")
+	fs.IntVar(&cfg.AutoCompactionMode, "auto-compaction-mode", 0, "Interpreted 'auto-comapction-retention' as hours when 0, as revision numbers when 1.")


this should be a string taking some nice name for the policy, something like "hourly" (default) and "revision"

Since we might want to change the frequency of compaction by the periodic compactor, I think 'period' would be better here. If we use 'hourly', we cannot change the interval.
(To keep the storage size more consistent, it might be better for the periodic compactor to run compaction more often).

heyitsanthony · 2017-06-19T18:33:48Z

etcdserver/server.go

@@ -469,8 +469,15 @@ func NewServer(cfg ServerConfig) (srv *EtcdServer, err error) {
 		return nil, err
 	}
 	srv.authStore = auth.NewAuthStore(srv.be, tp)
-	if h := cfg.AutoCompactionRetention; h != 0 {
-		srv.compactor = compactor.NewPeriodic(h, srv.kv, srv)
+	if num := cfg.AutoCompactionRetention; num != 0 {


possibly have

srv.compactor = compactor.New(cfg.AutoCompactionMode, cfg.AutoCompactionRetention, srv.kv, srv)

so that the builder logic is kept with the compactor package

Done. Let me know if you'd like to define a type for the mode like type CompactionMode string.

yudai · 2017-06-20T00:21:42Z

To keep backward compatibility, I didn't change the type of retention hours from int to int64.
If the signature of Periodic is not so important (maybe it's used internal only), changing the type can make the code a little bit simpler without casting.

xiang90 · 2017-06-20T01:37:02Z

compactor/revisional.go

+				continue
+			}
+
+			plog.Noticef("Starting auto-compaction at revision %d", rev)


print out our retention here?

xiang90 · 2017-06-20T01:37:22Z

compactor/revisional.go

+	paused bool
+}
+
+func NewRevisional(retention int64, rg RevGetter, c Compactable) *Revisional {


document this please

xiang90 · 2017-06-20T01:37:34Z

compactor/periodic.go

+	paused bool
+}
+
+func NewPeriodic(h int, rg RevGetter, c Compactable) *Periodic {


add comments?

yudai · 2017-06-20T02:29:58Z

Done adding comments.
Also added some comments to Compactor interface as well.

gyuho · 2017-06-20T15:46:50Z

compactor/periodic.go

@@ -0,0 +1,122 @@
+// Copyright 2016 The etcd Authors


Changed to 2017.

heyitsanthony · 2017-06-20T20:46:20Z

compactor/compactor.go

@@ -32,8 +31,24 @@ var (
 const (
 	checkCompactionInterval   = 5 * time.Minute
 	executeCompactionInterval = time.Hour
+
+	ModePeriodic   = "period"


periodic instead?

Got it.
Should I rename the Revisional to Revision and ModeRevisonal = "revision" to ModeRevison = "revision"? mixing "revisional" and "revision" could be confusing.

ok, changed to Revision for consistency.

heyitsanthony · 2017-06-20T20:47:31Z

etcdmain/config.go

@@ -199,7 +199,8 @@ func newConfig() *config {
 	// version
 	fs.BoolVar(&cfg.printVersion, "version", false, "Print the version and exit.")

-	fs.IntVar(&cfg.AutoCompactionRetention, "auto-compaction-retention", 0, "Auto compaction retention for mvcc key value store in hour. 0 means disable auto compaction.")
+	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", "period", "Interpreted 'auto-comapction-retention' as hours when 'period', as revision numbers when 'revision'.")


Interpret 'auto-compaction-retention'...

heyitsanthony · 2017-06-20T20:49:27Z

lgtm after renaming the mode from period to periodic and squashing

yudai · 2017-06-20T21:19:01Z

Renamed and squashed!

heyitsanthony · 2017-06-20T23:07:25Z

embed/config.go

@@ -80,6 +80,7 @@ type Config struct {
 	Name                    string `json:"name"`
 	SnapCount               uint64 `json:"snapshot-count"`
 	AutoCompactionRetention int    `json:"auto-compaction-retention"`
+	AutoCompactionMode      string `json:"auto-compaction-mode`


missing end quote is causing CI to fail

heyitsanthony · 2017-06-20T23:08:26Z

please change the commit message's first line to *: add Revisional compactor so CI is happy about commit formatting. Thanks!

heyitsanthony · 2017-06-21T20:31:50Z

@yudai still failing CI on json:"auto-compaction-mode...

yudai · 2017-06-21T22:41:30Z

@heyitsanthony Oops, sorry about that. Now it should be fixed

xiang90 · 2017-06-22T23:34:23Z

CI failures are unrelated. merging. thanks! @yudai

yudai force-pushed the revision_compactor branch from 5f22d13 to e65d407 Compare June 17, 2017 00:49

yudai changed the title ~~Compactor: Add Revision compactor~~ Compactor: Add Revisional compactor Jun 17, 2017

heyitsanthony suggested changes Jun 19, 2017

View reviewed changes

yudai force-pushed the revision_compactor branch from e65d407 to 3b780e5 Compare June 20, 2017 00:14

xiang90 reviewed Jun 20, 2017

View reviewed changes

compactor/periodic.go Outdated

paused bool

}

func NewPeriodic(h int, rg RevGetter, c Compactable) *Periodic {

Copy link

Contributor

xiang90 Jun 20, 2017

Choose a reason for hiding this comment

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

add comments?

xiang90 added this to the v3.3.0 milestone Jun 20, 2017

yudai force-pushed the revision_compactor branch from 3b780e5 to 5d72e95 Compare June 20, 2017 02:29

yudai force-pushed the revision_compactor branch from 5d72e95 to 8b5f70b Compare June 20, 2017 02:31

gyuho reviewed Jun 20, 2017

View reviewed changes

gyuho added the kind/release-note label Jun 20, 2017

yudai force-pushed the revision_compactor branch from 8b5f70b to 057dc9f Compare June 20, 2017 20:29

heyitsanthony reviewed Jun 20, 2017

View reviewed changes

yudai force-pushed the revision_compactor branch from 057dc9f to e4192f8 Compare June 20, 2017 21:18

heyitsanthony reviewed Jun 20, 2017

View reviewed changes

yudai force-pushed the revision_compactor branch from e4192f8 to e38eb4c Compare June 21, 2017 06:55

*: add Revision compactor

a3f8f47

yudai force-pushed the revision_compactor branch from e38eb4c to a3f8f47 Compare June 21, 2017 22:41

heyitsanthony approved these changes Jun 21, 2017

View reviewed changes

xiang90 merged commit 0fe8fdc into etcd-io:master Jun 22, 2017

xiang90 mentioned this pull request Jun 22, 2017

revision based auto-compaction-retention #8098

Closed

r7vme mentioned this pull request Sep 22, 2017

update-etcd-rewrite giantswarm/k8scloudconfig#202

Merged

ah8ad3 mentioned this pull request Mar 26, 2024

Flaky test TestRevisionPause #17548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compactor: Add Revisional compactor #8123

Compactor: Add Revisional compactor #8123

yudai commented Jun 17, 2017 •

edited

Loading

heyitsanthony left a comment

heyitsanthony Jun 19, 2017

yudai Jun 20, 2017

heyitsanthony Jun 19, 2017

yudai Jun 20, 2017 •

edited

Loading

yudai commented Jun 20, 2017

xiang90 Jun 20, 2017

xiang90 Jun 20, 2017

xiang90 Jun 20, 2017

yudai commented Jun 20, 2017

gyuho Jun 20, 2017

yudai Jun 20, 2017

heyitsanthony Jun 20, 2017

yudai Jun 20, 2017

heyitsanthony Jun 20, 2017

yudai Jun 20, 2017

heyitsanthony Jun 20, 2017

heyitsanthony commented Jun 20, 2017

yudai commented Jun 20, 2017

heyitsanthony Jun 20, 2017 •

edited

Loading

heyitsanthony commented Jun 20, 2017

heyitsanthony commented Jun 21, 2017

yudai commented Jun 21, 2017

xiang90 commented Jun 22, 2017

Compactor: Add Revisional compactor #8123

Compactor: Add Revisional compactor #8123

Conversation

yudai commented Jun 17, 2017 • edited Loading

heyitsanthony 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

yudai Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

yudai commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yudai commented Jun 20, 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

heyitsanthony commented Jun 20, 2017

yudai commented Jun 20, 2017

heyitsanthony Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

heyitsanthony commented Jun 20, 2017

heyitsanthony commented Jun 21, 2017

yudai commented Jun 21, 2017

xiang90 commented Jun 22, 2017

yudai commented Jun 17, 2017 •

edited

Loading

yudai Jun 20, 2017 •

edited

Loading

heyitsanthony Jun 20, 2017 •

edited

Loading