mvcc/kvstore: Optimize compaction

[shenjiangc]

mvcc/kvstore: Optimize compaction

when the number of key-value index is greater than one million, compact take too long and blocks other requests.
Move the “kvindex.Compact” to “fifoSched.Schedule” func

[jingyih]

Could you elaborate how this change helps? This is still the same amount of work but is done at a different time? Do you have test data showing the improvement? Thanks!

Also if we move index compaction to the scheduled FIFO, we need to adjust the metric "indexCompactionPauseMs" accordingly.

[shenjiangc]

Could you elaborate how this change helps? This is still the same amount of work but is done at a different time? Do you have test data showing the improvement? Thanks!

Also if we move index compaction to the scheduled FIFO, we need to adjust the metric "indexCompactionPauseMs" accordingly.

Yes, Just let index compact do in another goroutine。

first，I put 1000000 or more key-value：
./tools/benchmark --target-leader --conns=50 --clients=50 put --key-size=64 --sequential-keys --total=5000000 --val-size=256 --key-space-size=5000000

then，i do compact use：
rev=$(./etcdctl --endpoints=http://127.0.0.1:2379 endpoint status --write-out="json" | egrep -o '"revision":[0-9]' | egrep -o '[0-9].') ; echo $rev
./etcdctl --endpoints=http://127.0.0.1:2379 compact $rev
the compaction took more than 1 sec。

when the compact is going on， the other new put request is blocked, and Completion delay > 1 sec. The reason is the index compact took too time to assign map[]，when the tree is big enough。

Move index compaction to the scheduled FIFO， let the compaction don't block the new request。

TEST ：（put 5 key，when compacting）
Before Modification：
./tools/benchmark --target-leader --conns=1 --clients=1 put --key-size=64 --sequential-keys --total=5 --val-size=256 --key-space-size=5
Summary:
Total: 2.9459 secs.
Slowest: 2.9445 secs.
Fastest: 0.0002 secs.
Average: 0.5892 secs.
Stddev: 1.1776 secs.
Requests/sec: 1.6973
Response time histogram:
0.0002 [1] |∎∎∎∎∎∎∎∎∎∎∎∎∎
0.2947 [3] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
0.5891 [0] |
0.8835 [0] |
1.1779 [0] |
1.4723 [0] |
1.7668 [0] |
2.0612 [0] |
2.3556 [0] |
2.6500 [0] |
2.9445 [1] |∎∎∎∎∎∎∎∎∎∎∎∎∎

After Modification：
./tools/benchmark --target-leader --conns=1 --clients=1 put --key-size=64 --sequential-keys --total=5 --val-size=256 --key-space-size=5
Summary:
Total: 0.0028 secs.
Slowest: 0.0014 secs.
Fastest: 0.0003 secs.
Average: 0.0005 secs.
Stddev: 0.0004 secs.
Requests/sec: 1783.4436

Response time histogram:
0.0003 [1] |∎∎∎∎∎∎∎∎∎∎∎∎∎
0.0004 [3] |∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎∎
0.0005 [0] |
0.0006 [0] |
0.0007 [0] |
0.0009 [0] |
0.0010 [0] |
0.0011 [0] |
0.0012 [0] |
0.0013 [0] |
0.0014 [1] |∎∎∎∎∎∎∎∎∎∎∎∎∎

[jingyih]

Thanks for doing benchmark testing!

With this change, when the actual index compaction happens later, it will still block other requests such as put request. Is my understanding correct?

[shenjiangc]

With this modification, compaction will no longer block other request messages。;)

[jingyih]

With this modification, compaction will no longer block other request messages。;)

Sorry I don't quite understand. This modification basically postponed the compaction work in index tree to a later time. So I can understand the compaction request does not block other requests immediately. But later when the actual compaction work happens, it has the same effect on other requests, right?

[shenjiangc]

Hi,
The etcd process normal request messages such as put/get/compact in the same goroute “go run()”. When it is processing compact,it will can’t process other messages. The fifo scheduler is another goroute. So I move the index compact to fifo scheduler, to let the goroute “go run()” to process the next request as soon.
I tested it，when the actual compaction happen,it doesn’t block the other request.

[jingyih]

Thanks I see your point. My previous thinking was that the index compaction does hold a write lock most of the time during its execution, so it blocks other request (which also need read or write the same index tree) anyway.

Another potential concern is the function returns w/o actually doing the compaction work in index tree (because it is just scheduled). So depends on when the index compaction happens, the returned "keep" could be different, meaning the keys to be deleted in the data base is different. I don't think there are any correctness concerns, but it makes consistency checking among nodes more difficult. - not completely sure about what I said here, need some time to read and think through.

[jingyih]

So depends on when the index compaction happens, the returned "keep" could be different, meaning the keys to be deleted in the data base is different.

I think my previous statement was wrong. "keep" does not depend on key modifications after compaction rev.

So this should be safe.

[jingyih]

LGTM

[jingyih]

This conflicts with the tracing we recently added in #11179.

@YoyinZyc Could you help take a look? If we move the compaction of in-memory index to scheduled, can we still time it? Or maybe we need to drop the corresponding step?

trace.Step("compact in-memory index tree")

[jingyih]

Oh actually we also need to adjust the calculation of metrics indexCompactionPauseMs.

[YoyinZyc]

This conflicts with the tracing we recently added in #11179.

Yuchen Zhou Could you help take a look? If we move the compaction of in-memory index to scheduled, can we still time it? Or maybe we need to drop the corresponding step?

trace.Step("compact in-memory index tree")

We can remove this step in that case. It will be timed with the backend together as a whole scheduled job.

[jingyih]

@shenjiangc Could you please rebase to current master? And then:

1. remove trace.Step("compact in-memory index tree").
2. move the calculation of "indexCompactionPauseMs" to the scheduled function. It is used to measure the duration of s.kvindex.Compact(rev).

[shenjiangc]

@shenjiangc Could you please rebase to current master? And then:

1. remove trace.Step("compact in-memory index tree").
2. move the calculation of "indexCompactionPauseMs" to the scheduled function. It is used to measure the duration of s.kvindex.Compact(rev).

I made some mistacks, the new pull request is #11330 , Please take a look。