[release-3.5] Fix a performance regression due to uncertain compaction sleep interval #19405

miancheng7 · 2025-02-12T22:39:46Z

The compaction behavior here is changed in commit
02635 and introduces a latency issue. To be more speicific, the ticker.C acts as a fixed timer that triggers every 10ms, regardless of how long each batch of compaction takes. This means that if a previous compaction batch takes longer than 10ms, the next batch starts immediately, making compaction a blocking operation for etcd.

To fix the issue, this commit revert the compaction to the previous behavior which ensures a 10ms delay between each batch of compaction, allowing other read and write operations to proceed smoothly.

issue #19406

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

k8s-ci-robot · 2025-02-12T22:39:56Z

Hi @miancheng7. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

The compaction behavior is changed in commit [02635](etcd-io@0263597) and introduces a latency issue. To be more speicific, the `ticker.C` acts as a fixed timer that triggers every 10ms, regardless of how long each batch of compaction takes. This means that if a previous compaction batch takes longer than 10ms, the next batch starts immediately, making compaction a blocking operation for etcd. To fix the issue, this commit revert the compaction to the previous behavior which ensures a 10ms delay between each batch of compaction, allowing other read and write operations to proceed smoothly. Signed-off-by: Miancheng Lin <iml@amazon.com>

miancheng7 · 2025-02-12T22:56:15Z

cc @chaochn47 for visibility

hakuna-matatah · 2025-02-12T23:26:25Z

server/mvcc/kvstore_compaction.go

@@ -90,7 +88,7 @@ func (s *store) scheduleCompaction(compactMainRev, prevCompactRev int64) (KeyVal
 		dbCompactionPauseMs.Observe(float64(time.Since(start) / time.Millisecond))

 		select {
-		case <-batchTicker.C:
+		case <-time.After(s.cfg.CompactionSleepInterval):


While I understand you are doing a revert of the change.

Was wondering couple of things:

Curious why its defaulted to 10ms today in the first place ?

As I understand it, compaction is taking longer than 10ms in clusters with large data sets, leading to noticeable latency increases on client side (that's how this issue is surfaced iiuc).
So, I was wondering if we can adjust sleep interval dynamically based on pending apply requests instead of creating a room for 10ms ?. The more apply requests pending, the longer the sleep interval, thus reducing client latency(for reads/writes) during high load scenarios and could be beneficial in large clusters.
wdyt ?

For q1, 10ms is introduced by #11034, and it seems like an empirical param #11021 (comment) (not the PR author's comment)

For question 2, regardless of the duration of a compaction batch, the current implementation does not guarantee a full CompactionSleepInterval sleep. Given that CompactionSleepInterval is already configurable, I think we could address this issue first and then introduce an adaptive sleep mode in a separate pull request 🤔

For backport please stick to minimal change that will address the issue

+1 to keep the minimal change in this PR.

For any improvement, let's address & discuss it separately.

Yes, I am totally all for merging this for sure to fix the problem at hand.

let's address & discuss it separately.

I just left the community to understand what community thinks about this to see if it makes sense to go in that direction ? I can probably cut a separate issue to channel this discussion.

Sorry, do not get time to dig into your proposal yet. Please feel free to raise a separate ticket to present your thought/idea if you want. But you need to convince us that it's indeed a real issue and demo the real benefit to go in that direction. Overall I do not see a high priority to change it for now.

serathius · 2025-02-13T08:44:38Z

Please start from sending PR to the main branch, get it merged and then backport to release-3.6 and release3.5 branches.

serathius · 2025-02-13T08:45:51Z

Would be also nice to have a test/benchmark to confirm fix and prevent regression in the future.

ahrtr

LGTM

Thank you!

ahrtr · 2025-02-13T09:33:55Z

server/mvcc/kvstore_compaction.go

@@ -90,7 +88,7 @@ func (s *store) scheduleCompaction(compactMainRev, prevCompactRev int64) (KeyVal
 		dbCompactionPauseMs.Observe(float64(time.Since(start) / time.Millisecond))

 		select {
-		case <-batchTicker.C:
+		case <-time.After(s.cfg.CompactionSleepInterval):


+1 to keep the minimal change in this PR.

For any improvement, let's address & discuss it separately.

ahrtr · 2025-02-13T09:34:31Z

/ok-to-test

ahrtr · 2025-02-13T09:34:55Z

Would be also nice to have a test/benchmark to confirm fix and prevent regression in the future.

+1.

Sorry. I did not notice that this PR is on 3.5

ahrtr · 2025-02-13T09:37:34Z

Let me send a PR to main now.

ahrtr · 2025-02-13T09:41:48Z

Link to the PR on main #19410

ahrtr · 2025-02-13T13:54:47Z

cc @fuweid @serathius

fuweid

Changes looks good.

Just thinking how to handle backport in the future. Since we have merged the change into main branch, should we cherry-pick it or just merge this one? My suggestion is that we should address issue in main branch and then backport if applicable.

However, this change came first. We can merge this.

k8s-ci-robot · 2025-02-13T14:02:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, fuweid, miancheng7

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ahrtr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ahrtr · 2025-02-13T14:08:14Z

Just thinking how to handle backport in the future. Since we have merged the change into main branch, should we cherry-pick it or just merge this one?

Usually fix goes into main firstly, then backport to stable releases. But not a big problem. It's OK as long as we ensure the fix is merged on higher versions firstly.

miancheng7 · 2025-02-13T18:46:32Z

Usually fix goes into main firstly, then backport to stable releases. But not a big problem. It's OK as long as we ensure the fix is merged on higher versions firstly.

Thanks for the information. Will go with this approach next time.

k8s-ci-robot added the needs-ok-to-test label Feb 12, 2025

k8s-ci-robot added the size/XS label Feb 12, 2025

miancheng7 mentioned this pull request Feb 12, 2025

Performance regression in etcd v3.5.16 #19406

Open

4 tasks

miancheng7 force-pushed the fix-compaction-induce-latency branch from 07e9fd6 to da930c7 Compare February 12, 2025 22:55

hakuna-matatah reviewed Feb 12, 2025

View reviewed changes

ahrtr approved these changes Feb 13, 2025

View reviewed changes

k8s-ci-robot added the approved label Feb 13, 2025

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Feb 13, 2025

ahrtr changed the title ~~fix a compaction induce latency issue~~ [release-3.5] fix a compaction induce latency issue Feb 13, 2025

ahrtr changed the title ~~[release-3.5] fix a compaction induce latency issue~~ [release-3.5] Fix a performance regression due to uncertain compaction sleep interval Feb 13, 2025

ahrtr mentioned this pull request Feb 13, 2025

Add the fix of the performance regression due to uncertain compaction… #19412

Merged

fuweid approved these changes Feb 13, 2025

View reviewed changes

ahrtr merged commit 4fb86eb into etcd-io:release-3.5 Feb 13, 2025
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release-3.5] Fix a performance regression due to uncertain compaction sleep interval #19405

[release-3.5] Fix a performance regression due to uncertain compaction sleep interval #19405

miancheng7 commented Feb 12, 2025 •

edited

Loading

k8s-ci-robot commented Feb 12, 2025

miancheng7 commented Feb 12, 2025

hakuna-matatah Feb 12, 2025 •

edited

Loading

JalinWang Feb 13, 2025

JalinWang Feb 13, 2025 •

edited

Loading

serathius Feb 13, 2025

ahrtr Feb 13, 2025

hakuna-matatah Feb 13, 2025

ahrtr Feb 13, 2025

serathius commented Feb 13, 2025

serathius commented Feb 13, 2025 •

edited

Loading

ahrtr left a comment

ahrtr Feb 13, 2025

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

fuweid left a comment

k8s-ci-robot commented Feb 13, 2025

ahrtr commented Feb 13, 2025

miancheng7 commented Feb 13, 2025

[release-3.5] Fix a performance regression due to uncertain compaction sleep interval #19405

[release-3.5] Fix a performance regression due to uncertain compaction sleep interval #19405

Conversation

miancheng7 commented Feb 12, 2025 • edited Loading

k8s-ci-robot commented Feb 12, 2025

miancheng7 commented Feb 12, 2025

hakuna-matatah Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

JalinWang Feb 13, 2025

Choose a reason for hiding this comment

JalinWang Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

serathius Feb 13, 2025

Choose a reason for hiding this comment

ahrtr Feb 13, 2025

Choose a reason for hiding this comment

hakuna-matatah Feb 13, 2025

Choose a reason for hiding this comment

ahrtr Feb 13, 2025

Choose a reason for hiding this comment

serathius commented Feb 13, 2025

serathius commented Feb 13, 2025 • edited Loading

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr Feb 13, 2025

Choose a reason for hiding this comment

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

ahrtr commented Feb 13, 2025

fuweid left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 13, 2025

ahrtr commented Feb 13, 2025

miancheng7 commented Feb 13, 2025

miancheng7 commented Feb 12, 2025 •

edited

Loading

hakuna-matatah Feb 12, 2025 •

edited

Loading

JalinWang Feb 13, 2025 •

edited

Loading

serathius commented Feb 13, 2025 •

edited

Loading