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: Add Revisional compactor #8123

Merged
merged 1 commit into from
Jun 22, 2017
Merged

Conversation

yudai
Copy link
Contributor

@yudai yudai commented Jun 17, 2017

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.

@yudai yudai changed the title Compactor: Add Revision compactor Compactor: Add Revisional compactor Jun 17, 2017
Copy link
Contributor

@heyitsanthony heyitsanthony left a comment

Choose a reason for hiding this comment

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

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

@@ -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.")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly have

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

so that the builder logic is kept with the compactor package

Copy link
Contributor Author

@yudai yudai Jun 20, 2017

Choose a reason for hiding this comment

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

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

@yudai
Copy link
Contributor Author

yudai commented Jun 20, 2017

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.

continue
}

plog.Noticef("Starting auto-compaction at revision %d", rev)
Copy link
Contributor

Choose a reason for hiding this comment

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

print out our retention here?

paused bool
}

func NewRevisional(retention int64, rg RevGetter, c Compactable) *Revisional {
Copy link
Contributor

Choose a reason for hiding this comment

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

document this please

paused bool
}

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

Choose a reason for hiding this comment

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

add comments?

@xiang90 xiang90 added this to the v3.3.0 milestone Jun 20, 2017
@yudai
Copy link
Contributor Author

yudai commented Jun 20, 2017

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

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

Choose a reason for hiding this comment

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

2017?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to 2017.

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

ModePeriodic = "period"
Copy link
Contributor

Choose a reason for hiding this comment

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

periodic instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

up to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, changed to Revision for consistency.

@@ -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'.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Interpret 'auto-compaction-retention'...

@heyitsanthony
Copy link
Contributor

lgtm after renaming the mode from period to periodic and squashing

@yudai
Copy link
Contributor Author

yudai commented Jun 20, 2017

Renamed and squashed!

embed/config.go Outdated
@@ -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`
Copy link
Contributor

@heyitsanthony heyitsanthony Jun 20, 2017

Choose a reason for hiding this comment

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

missing end quote is causing CI to fail

@heyitsanthony
Copy link
Contributor

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

@heyitsanthony
Copy link
Contributor

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

@yudai
Copy link
Contributor Author

yudai commented Jun 21, 2017

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

@xiang90
Copy link
Contributor

xiang90 commented Jun 22, 2017

CI failures are unrelated. merging. thanks! @yudai

@xiang90 xiang90 merged commit 0fe8fdc into etcd-io:master Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants