kvserver: improve help text for `admission.io.overload` by irfansharif · Pull Request #90788 · cockroachdb/cockroach

irfansharif · 2022-10-27T18:31:16Z

Since this is text that's available in any running binary (SHOW ALL CLUSTER SETTINGS), we shouldn't expect readers to have the source code checked out, which makes our referencing of specific file names less than useful. See #87656 (review).

Release note: None
Release justification: No behavioural change, only changing user-visible help text.

cockroach-teamcity · 2022-10-27T18:31:28Z

This change is

KaiSun314

Thank you for improving the help text for admission.io.overload!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @irfansharif and @kvoli)

pkg/kv/kvserver/metrics.go line 1063 at r1 (raw file):

		Help: `1-normalized float indicating whether IO admission control considers the store as overloaded with respect to compaction out of L0 (considers sub-level and file counts).`
		Measurement: "Threshold",
		Unit:        metric.Unit_PERCENT,

The range for admission io overload metric is technically [0, +∞) (albeit most of the time this metric should be within [0, 1)). Moreover, we compare this metric against the threshold set by the admission.kv.pause_replication_io_threshold cluster setting, which may not be 1. I was wondering if Unit_PERCENT would still be proper if admission io overload metric could be > 1?

KaiSun314

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @irfansharif and @kvoli)

pkg/kv/kvserver/metrics.go line 1061 at r1 (raw file):

	metaIOOverload = metric.Metadata{
		Name: "admission.io.overload",
		Help: `1-normalized float indicating whether IO admission control considers the store as overloaded with respect to compaction out of L0 (considers sub-level and file counts).`

Need comma at end of line

Since this is text that's available in any running binary (`SHOW ALL CLUSTER SETTINGS`), we shouldn't expect readers to have the source code checked out, which makes our referencing of specific file names less than useful. Release note: None Release justification: No behavioural change, only changing user-visible help text.

irfansharif

Thanks!

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @KaiSun314 and @kvoli)

pkg/kv/kvserver/metrics.go line 1061 at r1 (raw file):

Previously, KaiSun314 (Kai Sun) wrote…

Need comma at end of line

Done.

pkg/kv/kvserver/metrics.go line 1063 at r1 (raw file):
Unit percent is fine, it's sensible to say we're 150% of the threshold since AC is off/malfunctioning for ex.

Moreover, we compare this metric against the threshold set by the admission.kv.pause_replication_io_threshold cluster setting, which may not be 1

A lot of our % based controls/cluster settings are floating point ones, so that feels fine too. These for ex.:

cockroach/pkg/util/admission/scheduler_latency_listener.go

Lines 209 to 260 in e397faf

    
           var ( // cluster settings to control how elastic CPU % is adjusted 
        
           	elasticCPUMaxUtilization = settings.RegisterFloatSetting( 
        
           		settings.SystemOnly, 
        
           		"admission.elastic_cpu.max_utilization", 
        
           		"sets the ceiling on per-node elastic work CPU % utilization", 
        
           		0.75, // 75% 
        
           		func(f float64) error { 
        
           			if f < 0.05 || f > 1.0 { 
        
           				return fmt.Errorf("expected max_utilization to be between [0.0, 1.0], got %0.2f", f) 
        
           			} 
        
           			return nil 
        
           		}, 
        
           	) 
        
           	elasticCPUMinUtilization = settings.RegisterFloatSetting( 
        
           		settings.SystemOnly, 
        
           		"admission.elastic_cpu.min_utilization", 
        
           		"sets the floor on per-node elastic work CPU % utilization", 
        
           		0.05, // 5% 
        
           		func(f float64) error { 
        
           			if f < 0.01 || f > 1.0 { 
        
           				return fmt.Errorf("expected min_utilization to be between [0.01, 1.0], got %0.2f", f) 
        
           			} 
        
           			return nil 
        
           		}, 
        
           	) 
        
           	elasticCPUInactivePoint = settings.RegisterFloatSetting( 
        
           		settings.SystemOnly, 
        
           		"admission.elastic_cpu.inactive_point", 
        
           		"the point between {min,max}_utilization the CPU % decreases to when there's no elastic work", 
        
           		0.10, // 10% of the way between {min,max} utilization -- 12% if [min,max] = [5%,75%] 
        
           		func(f float64) error { 
        
           			if f < 0.0 || f > 1.0 { 
        
           				return fmt.Errorf("expected inactive_point to be between [0.0, 1.0], got %0.2f", f) 
        
           			} 
        
           			return nil 
        
           		}, 
        
           	) 
        
           	elasticCPUAdjustmentDeltaPerSecond = settings.RegisterFloatSetting( 
        
           		settings.SystemOnly, 
        
           		"admission.elastic_cpu.adjustment_delta_per_second", 
        
           		"sets the per-second % adjustment used when when adapting elastic work CPU %s", 
        
           		0.001, // 0.1%, takes 10s to add 1% to elastic CPU limit 
        
           		func(f float64) error { 
        
           			if f < 0.0001 || f > 1.0 { 
        
           				return fmt.Errorf("expected additive_delta_per_second to be between [0.0001, 1.0], got %0.2f", f) 
        
           			} 
        
           			return nil 
        
           		}, 
        
           	)

craig · 2022-10-27T19:44:26Z

👎 Rejected by code reviews

irfansharif · 2022-10-27T19:44:50Z

Oh RIP, @KaiSun314, you might have re-stamp.

KaiSun314

Oop sorry about that, thanks again for making this improvement!

Reviewable status: complete! 1 of 0 LGTMs obtained

irfansharif · 2022-10-27T22:18:40Z

Nothing to apologize for!

bors r+

craig · 2022-10-27T23:44:24Z

Build succeeded:

Bazel Essential CI (Cockroach)

irfansharif added the backport-22.2.x label Oct 27, 2022

irfansharif requested review from KaiSun314 and kvoli October 27, 2022 18:31

irfansharif requested a review from a team as a code owner October 27, 2022 18:31

KaiSun314 approved these changes Oct 27, 2022

View reviewed changes

KaiSun314 suggested changes Oct 27, 2022

View reviewed changes

irfansharif force-pushed the 221027.ac-io-thresh branch from 91ecde2 to 084f85a Compare October 27, 2022 19:38

irfansharif commented Oct 27, 2022

View reviewed changes

irfansharif requested a review from KaiSun314 October 27, 2022 19:45

kvoli approved these changes Oct 27, 2022

View reviewed changes

KaiSun314 approved these changes Oct 27, 2022

View reviewed changes

craig bot merged commit 6fba541 into cockroachdb:master Oct 27, 2022

blathers-crl bot mentioned this pull request Oct 27, 2022

release-22.2: kvserver: improve help text for admission.io.overload #90814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: improve help text for `admission.io.overload`#90788

kvserver: improve help text for `admission.io.overload`#90788
craig[bot] merged 1 commit intocockroachdb:masterfrom
irfansharif:221027.ac-io-thresh

irfansharif commented Oct 27, 2022 •

edited

Loading

Uh oh!

cockroach-teamcity commented Oct 27, 2022

Uh oh!

KaiSun314 left a comment

Uh oh!

KaiSun314 left a comment

Uh oh!

irfansharif left a comment

Uh oh!

craig bot commented Oct 27, 2022

Uh oh!

irfansharif commented Oct 27, 2022

Uh oh!

KaiSun314 left a comment

Uh oh!

irfansharif commented Oct 27, 2022

Uh oh!

craig bot commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	var ( // cluster settings to control how elastic CPU % is adjusted
	elasticCPUMaxUtilization = settings.RegisterFloatSetting(
	settings.SystemOnly,
	"admission.elastic_cpu.max_utilization",
	"sets the ceiling on per-node elastic work CPU % utilization",
	0.75, // 75%
	func(f float64) error {
	if f < 0.05 \|\| f > 1.0 {
	return fmt.Errorf("expected max_utilization to be between [0.0, 1.0], got %0.2f", f)
	}
	return nil
	},
	)

	elasticCPUMinUtilization = settings.RegisterFloatSetting(
	settings.SystemOnly,
	"admission.elastic_cpu.min_utilization",
	"sets the floor on per-node elastic work CPU % utilization",
	0.05, // 5%
	func(f float64) error {
	if f < 0.01 \|\| f > 1.0 {
	return fmt.Errorf("expected min_utilization to be between [0.01, 1.0], got %0.2f", f)
	}
	return nil
	},
	)

	elasticCPUInactivePoint = settings.RegisterFloatSetting(
	settings.SystemOnly,
	"admission.elastic_cpu.inactive_point",
	"the point between {min,max}_utilization the CPU % decreases to when there's no elastic work",
	0.10, // 10% of the way between {min,max} utilization -- 12% if [min,max] = [5%,75%]
	func(f float64) error {
	if f < 0.0 \|\| f > 1.0 {
	return fmt.Errorf("expected inactive_point to be between [0.0, 1.0], got %0.2f", f)
	}
	return nil
	},
	)

	elasticCPUAdjustmentDeltaPerSecond = settings.RegisterFloatSetting(
	settings.SystemOnly,
	"admission.elastic_cpu.adjustment_delta_per_second",
	"sets the per-second % adjustment used when when adapting elastic work CPU %s",
	0.001, // 0.1%, takes 10s to add 1% to elastic CPU limit
	func(f float64) error {
	if f < 0.0001 \|\| f > 1.0 {
	return fmt.Errorf("expected additive_delta_per_second to be between [0.0001, 1.0], got %0.2f", f)
	}
	return nil
	},
	)

Conversation

irfansharif commented Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Oct 27, 2022

Uh oh!

KaiSun314 left a comment

Choose a reason for hiding this comment

Uh oh!

KaiSun314 left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif left a comment

Choose a reason for hiding this comment

Uh oh!

craig bot commented Oct 27, 2022

Uh oh!

irfansharif commented Oct 27, 2022

Uh oh!

KaiSun314 left a comment

Choose a reason for hiding this comment

Uh oh!

irfansharif commented Oct 27, 2022

Uh oh!

craig bot commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

irfansharif commented Oct 27, 2022 •

edited

Loading