WIP, RFC *: a new option for size based compaction

[mitake]

This commit adds a new option --snapshot-size for specifying size
based compaction. The option takes a number of bytes. If the
accumulated size of in memory log entries becomes larger than the
given size, etcd triggers compaction.

I'm still working on evaluation for understanding the performance affection from this option.

/cc @xiang90

[mitake]

related issue: #7162

[codecov-io]

Codecov Report

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

@@            Coverage Diff            @@
##             master    #7782   +/-   ##
=========================================
  Coverage          ?   75.71%           
=========================================
  Files             ?      341           
  Lines             ?    26590           
  Branches          ?        0           
=========================================
  Hits              ?    20133           
  Misses            ?     5011           
  Partials          ?     1446

Impacted Files	Coverage Δ
etcdserver/config.go	77.77% <100%> (ø)
raft/storage.go	90.22% <100%> (ø)
embed/config.go	69.76% <100%> (ø)
etcdmain/config.go	79.75% <100%> (ø)
etcdserver/raft.go	87.87% <100%> (ø)
embed/etcd.go	73.7% <100%> (ø)
etcdserver/server.go	80.09% <88.88%> (ø)

---

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 8fbf7ce...503f9e1. Read the comment docs.

[mitake]

@xiang90 I think this PR is ready to review. Could you take a look?

[xiang90]

the name is confusing. users will probably feel it is the actual snapshot file size. but the limit is actually the total size of in memory entries.

we need to come up with a better name. and, yes, snap-count is also a bad name in my opinion. it makes sense for v2, but not for v3.

[mitake]

@xiang90 I see, then how about --snapshot-trigger-log-size? Too long?

[mitake]

Probably I should cc @bdarnell because this PR relates with the raft package?

[bdarnell]

The changes to raft.MemoryStorage are OK with me (we don't use it in CockroachDB).

[mitake]

@bdarnell thanks for your review!

[heyitsanthony]

@mitake I would like to separate the policy from the rest of MemoryStorage, if at least to avoid the disruption from having to change the memory storage constructor. I think it can be done in the etcdserver package like:

type MemoryStorageCompactHinter interface {
    raft.Storage
    CompactIndex() int64
    CreateSnapshot(...) // + anything else needed from memorystorage not in the storage interface
}

type storageCompactHintNone struct { *raft.MemoryStorage }

func (nh *storageCompactHintNone) CompactIndex() int64 {
    // no-op compaction
    v, err := nh.FirstIndex()
    if err != nil { panic(err) }
    return v
}

type storageCompatHintSize struct {
    *raft.MemoryStorage
    entsSize uint64
    snapSize uint64
    snapSizeCompactIdx int64
}

...

func newMemoryStorage(ms *raft.MemoryStorage, sizeHint int) MemoryStorageCompactHinter {
    if sizeHint == 0 {
       return &storageCompactHintNone{ms}
    }
    return &storageCompactHintSize{ms, sizeHint}
}

where raftNodeConfig.raftStorage is changed to use MemoryStorageCompactHinter.

the size hinting storage implementation would need an extra lock, but I don't think the performance hit will be too bad

[mitake]

@heyitsanthony the separation seems to be good. I'll follow the design in the next update.

[mitake]

@heyitsanthony I found that implementing the memory storage w/ compaction hint only in etcdserver side is difficult. This is because information required for size based compaction depends on MemoryStorage internals like the length and the total size MemoryStorage.ents. Implementing the hint wrapper in raft side is ok?

[heyitsanthony]

@mitake OK, so long as entsSize etc are still in a structure separated from MemoryStorage like storageCompactHintSize

[mikelnrd]

Thank you! Spark by Readdle

[gyuho]

@mitake I am moving this after 3.3, unless there are noticeable performance gains.

[mitake]

@gyuho yes the moving is ok because it isn't emergent.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[chaochn47]

Hi @mitake Do you know why this promising approach went stale? It can provide a reliable mechanism to predict the raft log memory consumption beforehand when doing memory budget planning.

I want to collaborate with you and continue this effort if no huge blockers.

[mitake]

hi @chaochn47 so sorry for my very late response. Are you still interested in this? If so I can cooperate with you for revisiting the idea.

[chaochn47]

Hi @mitake No worry. I am happy to work with the idea towards implementation. How about I raise a issue in etcd-io/raft, demonstrate my understanding of the PR and go from there?

[mitake]

@chaochn47 That's great, but as described in the roadmap: #15499 current top priority of the project is correctness. So I think we should be careful for not compromising correctness for performance.
Could you open a discussion on raft repository side then?