etcdserver: rename defaultCompactionSleepInterval var #18495

JalinWang · 2024-08-26T03:18:38Z

Signed-off-by: Jalin Wang <JalinWang@outlook.com>

k8s-ci-robot · 2024-08-26T03:18:47Z

Hi @JalinWang. 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.

ivanvc · 2024-08-26T03:40:25Z

/ok-to-test

ivanvc · 2024-08-26T03:46:41Z

Thanks for the pull request, @JalinWang. It looks like you missed updating the value in server/storage/mvcc/kvstore_test.go. Can you please update it, too?

@ahrtr, you suggested the variable be named defaultBatchInterval. Are you okay with defaultCompactionSleepInterval?

codecov-commenter · 2024-08-26T03:59:04Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 42.73504% with 201 lines in your changes missing coverage. Please review.

Project coverage is 68.80%. Comparing base (21e5876) to head (d9c8401).
Report is 188 commits behind head on main.

❗ Current head d9c8401 differs from pull request most recent head 9db3bd3

Please upload reports for the commit 9db3bd3 to get more accurate results.

Files	Patch %	Lines
client/v3/kubernetes/client.go	0.00%	91 Missing ⚠️
server/embed/config_logging.go	11.11%	23 Missing and 1 partial ⚠️
etcdctl/ctlv3/command/get_command.go	0.00%	18 Missing ⚠️
client/v3/maintenance.go	13.33%	13 Missing ⚠️
server/etcdserver/server.go	69.23%	8 Missing and 4 partials ⚠️
etcdctl/ctlv3/command/snapshot_command.go	0.00%	8 Missing ⚠️
client/v3/auth.go	66.66%	6 Missing ⚠️
client/v3/lease.go	40.00%	6 Missing ⚠️
pkg/cobrautl/help.go	76.47%	4 Missing ⚠️
server/embed/config.go	88.57%	2 Missing and 2 partials ⚠️
... and 8 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
client/v3/client.go	`84.89% <100.00%> (ø)`
client/v3/kv.go	`94.44% <100.00%> (ø)`
client/v3/txn.go	`100.00% <100.00%> (ø)`
client/v3/watch.go	`93.83% <100.00%> (ø)`
pkg/wait/wait_time.go	`100.00% <ø> (ø)`
server/auth/simple_token.go	`88.46% <100.00%> (ø)`
server/config/config.go	`79.76% <ø> (ø)`
server/etcdserver/api/v3rpc/health.go	`57.69% <100.00%> (ø)`
server/etcdserver/raft.go	`87.16% <100.00%> (ø)`
server/storage/mvcc/key_index.go	`64.70% <100.00%> (ø)`
... and 19 more

... and 391 files with indirect coverage changes

@@            Coverage Diff            @@
##           main   #18495       +/-   ##
=========================================
+ Coverage      0   68.80%   +68.80%     
=========================================
  Files         0      420      +420     
  Lines         0    35489    +35489     
=========================================
+ Hits          0    24418    +24418     
- Misses        0     9642     +9642     
- Partials      0     1429     +1429

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21e5876...9db3bd3. Read the comment docs.

JalinWang · 2024-08-26T05:57:07Z

Thanks for the pull request, @JalinWang. It looks like you missed updating the value in server/storage/mvcc/kvstore_test.go. Can you please update it, too?

Apologies :( I overlooked its usage in later commits.

@ahrtr, you suggested the variable be named defaultBatchInterval. Are you okay with defaultCompactionSleepInterval?

Following cfg.CompactionBatchLimit = defaultCompactBatchLimit, I think it's better to use defaultCompactionSleepInterval instead of the suggested defaultBatchInterval for cfg.CompactionSleepInterval.

update: emmm cfg.Compact**ion**BatchLimit -> defaultCompactBatchLimit... Should I make them the same? 🫤

Signed-off-by: Jalin Wang <JalinWang@outlook.com>

JalinWang · 2024-08-26T06:05:45Z

/retest

JalinWang · 2024-08-26T06:28:48Z

/retest

JalinWang · 2024-08-26T06:30:25Z

/retest-required

ahrtr · 2024-08-26T07:36:49Z

update: emmm cfg.Compact**ion**BatchLimit -> defaultCompactBatchLimit... Should I make them the same? 🫤

OK to me.

ahrtr · 2024-08-26T08:42:56Z

server/storage/mvcc/kvstore.go

@@ -40,7 +40,7 @@ var (

 var restoreChunkKeys = 10000 // non-const for testing
 var defaultCompactBatchLimit = 1000


nit as you suggested

Suggested change

var defaultCompactBatchLimit = 1000

var defaultCompactionBatchLimit = 1000

Signed-off-by: Jalin Wang <JalinWang@outlook.com>

ahrtr · 2024-08-27T13:21:00Z

Please squash the commits.

ahrtr · 2024-08-27T13:33:29Z

Please squash the commits.

Let me do it this time.

k8s-ci-robot · 2024-08-27T13:34:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, JalinWang, serathius

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,serathius]

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

* etcdserver: rename `minimumBatchInterval` to `defaultCompactionSleepInterval` and `defaultCompactBatchLimit` to `defaultCompactionBatchLimit` Signed-off-by: Jalin Wang <JalinWang@outlook.com> (cherry picked from commit 2c53be7)

This is a backporting cherry-pick of the following commits: - add flagsline Signed-off-by: Jalin Wang <JalinWang@outlook.com> - etcdserver: rename defaultCompactionSleepInterval var (etcd-io#18495) etcdserver: rename `minimumBatchInterval` to `defaultCompactionSleepInterval` and `defaultCompactBatchLimit` to `defaultCompactionBatchLimit` Signed-off-by: Jalin Wang <JalinWang@outlook.com> (cherry picked from commit 2c53be7) - test: add CompactionSleepInterval in FakeStore's config After setting the ComparionSleepInterval, we can use time.Ticker instead of time.After to optimize the scheduleComparison(), otherwise it will fail in the 'TestStoreCompact(t)' test. Signed-off-by: guozhao <guozhao@360.cn> (cherry picked from commit fab8474) - add sleep interval (cherry picked from commit 184b0e5) Signed-off-by: Jalin Wang <JalinWang@outlook.com>

* etcdserver: rename `minimumBatchInterval` to `defaultCompactionSleepInterval` and `defaultCompactBatchLimit` to `defaultCompactionBatchLimit` Signed-off-by: Jalin Wang <JalinWang@outlook.com>

etcdserver: rename defaultCompactionSleepInterval var

f18cf91

Signed-off-by: Jalin Wang <JalinWang@outlook.com>

k8s-ci-robot added the needs-ok-to-test label Aug 26, 2024

k8s-ci-robot added the size/XS label Aug 26, 2024

JalinWang mentioned this pull request Aug 26, 2024

add bolt compaction sleep interval #13018

Merged

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Aug 26, 2024

ivanvc mentioned this pull request Aug 26, 2024

Plan to release etcd v3.5.16 #18485

Closed

2 tasks

fix test

7bcc6fc

Signed-off-by: Jalin Wang <JalinWang@outlook.com>

serathius approved these changes Aug 26, 2024

View reviewed changes

k8s-ci-robot added the approved label Aug 26, 2024

ahrtr reviewed Aug 26, 2024

View reviewed changes

rename defaultCompactionBatchLimit

9db3bd3

Signed-off-by: Jalin Wang <JalinWang@outlook.com>

k8s-ci-robot added size/S and removed size/XS labels Aug 27, 2024

ahrtr approved these changes Aug 27, 2024

View reviewed changes

ahrtr merged commit 2c53be7 into etcd-io:main Aug 27, 2024
44 checks passed

JalinWang deleted the fix/rename_defaultBatchInterval branch September 9, 2024 02:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

etcdserver: rename defaultCompactionSleepInterval var #18495

etcdserver: rename defaultCompactionSleepInterval var #18495

JalinWang commented Aug 26, 2024

k8s-ci-robot commented Aug 26, 2024

ivanvc commented Aug 26, 2024

ivanvc commented Aug 26, 2024

codecov-commenter commented Aug 26, 2024 •

edited

Loading

JalinWang commented Aug 26, 2024 •

edited

Loading

JalinWang commented Aug 26, 2024

JalinWang commented Aug 26, 2024

JalinWang commented Aug 26, 2024

ahrtr commented Aug 26, 2024

ahrtr Aug 26, 2024

JalinWang Aug 27, 2024

ahrtr commented Aug 27, 2024

ahrtr commented Aug 27, 2024

k8s-ci-robot commented Aug 27, 2024

		@@ -40,7 +40,7 @@ var (

		var restoreChunkKeys = 10000 // non-const for testing
		var defaultCompactBatchLimit = 1000

	var defaultCompactBatchLimit = 1000
	var defaultCompactionBatchLimit = 1000

etcdserver: rename defaultCompactionSleepInterval var #18495

etcdserver: rename defaultCompactionSleepInterval var #18495

Conversation

JalinWang commented Aug 26, 2024

k8s-ci-robot commented Aug 26, 2024

ivanvc commented Aug 26, 2024

ivanvc commented Aug 26, 2024

codecov-commenter commented Aug 26, 2024 • edited Loading

Codecov Report

JalinWang commented Aug 26, 2024 • edited Loading

JalinWang commented Aug 26, 2024

JalinWang commented Aug 26, 2024

JalinWang commented Aug 26, 2024

ahrtr commented Aug 26, 2024

ahrtr Aug 26, 2024

Choose a reason for hiding this comment

JalinWang Aug 27, 2024

Choose a reason for hiding this comment

ahrtr commented Aug 27, 2024

ahrtr commented Aug 27, 2024

k8s-ci-robot commented Aug 27, 2024

codecov-commenter commented Aug 26, 2024 •

edited

Loading

JalinWang commented Aug 26, 2024 •

edited

Loading