Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: limit the frequency at which system config updates enqueue … #53603

Conversation

ajwerner
Copy link
Contributor

…replicas

This has a shockingly profound impact on the runtime of ORM tests.

This PR is my lower-risk, less fancy implementation of this functionality.
There is another inbound PR that permits bursting.

Release justification: low risk, high benefit changes to existing functionality

Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.

…replicas

This has a shockingly profound impact on the runtime of ORM tests.

This PR is my lower-risk, less fancy implementation of this functionality.
There is another inbound PR that permits bursting.

Release justification: low risk, high benefit changes to existing functionality

Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner
Copy link
Contributor Author

Prior to this change, over 70% of the CPU utilization of the typeorm roachtest was underneath those calls. With this 1s limit it comes down to about 3.8%. It is the difference between the test burning 4 cores fully vs barely utilizing 2 and decreases the runtime by 40%.

Screenshot_2020-08-28 cockroach cpu

@ajwerner ajwerner requested a review from tbg August 28, 2020 18:43
@ajwerner ajwerner marked this pull request as ready for review August 28, 2020 18:43
ajwerner added a commit to ajwerner/cockroach that referenced this pull request Aug 31, 2020
…replicas

This has a shockingly profound impact on the runtime of ORM tests.

This PR is a fancier implementation of cockroachdb#53603 that permits bursting and makes
setting a relatively low rate feel less risky in the face of a small burst of
changes.

Release justification: low risk, high benefit changes to existing functionality

Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.
@ajwerner
Copy link
Contributor Author

In practice I think #53605 is a much better change. This change is problematic in the face of small bursts of changes.

@ajwerner ajwerner marked this pull request as draft August 31, 2020 02:45
@ajwerner
Copy link
Contributor Author

Closing in favor of #53605.

@ajwerner ajwerner closed this Aug 31, 2020
craig bot pushed a commit that referenced this pull request Aug 31, 2020
53578: rfc: mark partial index RFC as complete r=rytaft a=mgartner

Release justification: This commit updates an RFC which is a
non-production code change.

Release note: None

53605: quotapool,kvserver: extend quotapool.RateLimit, rate limit queue addition in SystemConfigUpdate r=knz a=ajwerner

See individual commits. 

This PR is a fancier implementation of #53603 that permits bursting and makes
setting a relatively low rate feel less risky in the face of a small burst of
changes.

Release justification: low risk, high benefit changes to existing functionality
    
Release note (performance improvement): Limited the frequency of an expensive
operation due to schema changes making workload which perform schema changes
at a high rate less resource intensive.


53647: *: bump twpayne/go-geom@1.3.5, geos to include POINT EMPTY fix r=sumeerbhola a=otan

* Bumps twpayne/go-geom to resolve an issue with WKT encoding involving
  empty collection points.
* Bumps GEOS to handles POINT EMPTY correctly.

Resolves #53631.
Resolves #53632.

Release justification: low risk updates to new functionality

Release note: None

Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants