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

admission: carve out some new files #86065

Merged
merged 9 commits into from
Aug 19, 2022

Conversation

irfansharif
Copy link
Contributor

@irfansharif irfansharif commented Aug 12, 2022

Pure code movement. Trying to see what components in this package could be
reasoned about independently to readers less familiar with this package.
Some of these may benefit from being pulled into sub-packages, but for now
separate files are sufficient. We're hurting some git history tracking
with this PR, but hopefully they lead to more understandability with future
work. It's also better to do it now before the release branch is cut to
make backports easier. If we pull them into sub packages next, git tracking
will then come for free.

admission: move ioLoadListener into its own file
admission: move GrantCoordinator into its own file
admission: move kvSlotAdjuster into its own file
admission: move sqlNodeCPUOverloadIndicator into its own file
admission: move tokensLinearModel into its own file
admission: add top-level admission.go
admission: move store{AdmissionStats,RequestEstimates}..
admission: segment test files by central type

I'm not breaking things up into subpackages in this PR, but this paves the
way for that. The structure I'm imagining is roughly:

pkg/util/admission/
├── admission.go
├── admissionpb/
├── diskbandwidthlimiter/
├── diskloadwatcher/
├── grantcoordinator/
├── granter/
├── ioloadlistener/
├── kvslotadjuster/
├── sqlcpuoverloadindicator/
├── storeworkqueue/
└── tokenlinearmodel/

Where the sub-packages house concrete implementations of interfaces
defined in admission.go, corresponding tests, and constructors for those
types. They depend on the base level pkg/util/admission package and talk
to the other subpackages through interfaces. The existing interfaces are
already written in a manner to make this happen. For other examples of
this pattern, look at pkg/{upgrade,spanconfig}.

Release justification: low-risk / high-benefit (pure code movement, will make backports easier)

@irfansharif irfansharif requested a review from a team as a code owner August 12, 2022 22:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@irfansharif
Copy link
Contributor Author

I'm also adding @andrewbaptist since he's looking to better understand admission control going forward. In the near future we're hoping to use some of the disk bandwidth modelling we introduced in #85722 for snapshot admission as part of #80607.

Pure code movement change. Trying to see what components in this package
could be reasoned about independently to readers less familiar with this
package. Some of these may benefit from being pulled into sub-packages,
but for now a separate file is sufficient.

Release note: None
Also bring along with it some closely related structs:
StoreGrantCoordinators, Options, GrantCoordinatorMetrics. The latter was
previously named GranterMetrics, but this feels more appropriate. Pure
code movement change.

Release note: None
Pure code movement. We're hurting some git history tracking with these
code changes, but hopefully they lead to more understandability with
future work.

Release note: None
Pure code movement.

Release note: None
Subsuming doc.go and housing the central interfaces in this package. I'm
not breaking things up into subpackages in this PR, but this paves the
way for that. The structure I'm imagining is roughly:

    pkg/util/admission/
    ├── admission.go
    ├── admissionpb/
    ├── diskbandwidthlimiter/
    ├── diskloadwatcher/
    ├── grantcoordinator/
    ├── granter/
    ├── ioloadlistener/
    ├── kvslotadjuster/
    ├── sqlcpuoverloadindicator/
    ├── storeworkqueue/
    └── tokenlinearmodel/

Where the sub-packages house concrete implementations of interfaces
defined in admission.go, corresponding tests, and constructors for those
types. They depend on the base level pkg/util/admission package and talk
to the other subpackages through interfaces. The existing interfaces are
already written in a manner to make this happen. For other examples of
this pattern, look at pkg/{upgrade,spanconfig}.

Release note: None
..into work_queue.go, where they're primarily used. This feels more
suitable than granter.go.

Release note: None
@irfansharif irfansharif force-pushed the 220812.admission-file-mv branch 2 times, most recently from 40c1d53 to 58c8e8e Compare August 13, 2022 15:25
@tbg tbg removed their request for review August 15, 2022 12:42
@tbg
Copy link
Member

tbg commented Aug 15, 2022

Going to bow out of the review; between Andrew and Sumeer you should be covered. Thanks for splitting things apart, good idea.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r1, 4 of 4 files at r2, 3 of 3 files at r3, 3 of 3 files at r4, 3 of 3 files at r5, 5 of 5 files at r6, 2 of 2 files at r7, 6 of 6 files at r8, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist and @irfansharif)


pkg/util/admission/grant_coordinator_test.go line 30 at r8 (raw file):

// TestStoreCoordinators tests only the setup of GrantCoordinators per store.
// Testing of IO load functionality happens in TestIOLoadListener.

I am not sure about this separation. granter_test.go contains TestGranterBasic, which is also a GrantCoordinator test. We can keep this small test in that file. They also share testMetricsProvider.


pkg/util/admission/work_queue.go line 1560 at r7 (raw file):

}

// storeAdmissionStats are stats maintained by a storeRequester. The non-test

These two structs should go into admission.go since these are part of the contract defined by the interfaces there.
work_queue.go has implementers of the interfaces.

Pure code movement. They're part of the interface definition, so better
placed in the top-level admission.go housing the remaining interfaces.

Release note: None
Pure code movement change. Carve out
{io_load_listener,tokens_linear_model}_test.go using the same non-test
partitioning scheme as earlier commits. Within granter_test.go, move the
supporting types further below in the file.

Release note: None
Copy link
Contributor Author

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist and @sumeerbhola)


pkg/util/admission/grant_coordinator_test.go line 30 at r8 (raw file):

Previously, sumeerbhola wrote…

I am not sure about this separation. granter_test.go contains TestGranterBasic, which is also a GrantCoordinator test. We can keep this small test in that file. They also share testMetricsProvider.

Done.


pkg/util/admission/work_queue.go line 1560 at r7 (raw file):

Previously, sumeerbhola wrote…

These two structs should go into admission.go since these are part of the contract defined by the interfaces there.
work_queue.go has implementers of the interfaces.

Done, added a commit.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 8 files at r9, 7 of 7 files at r10.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @andrewbaptist and @sumeerbhola)

@irfansharif
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 19, 2022

Build succeeded:

@craig craig bot merged commit 677ef2c into cockroachdb:master Aug 19, 2022
@irfansharif irfansharif deleted the 220812.admission-file-mv branch August 19, 2022 18:33
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.

4 participants