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.

Sign up for GitHub

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

Jump to bottom

Move client-go/tools/record tests away from IntervalClock to SimpleIntervalClock #104578

Merged
merged 1 commit into from
Sep 20, 2021
Merged

Move client-go/tools/record tests away from IntervalClock to SimpleIntervalClock #104578

merged 1 commit into from
Sep 20, 2021

Conversation

MadhavJivrajani
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani commented Aug 25, 2021 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

  • Introduce PassiveRateLimiter which implements all methods of previous RateLimiter except Accept() and Wait()
  • Change RateLimiter interface to extend PassiveRateLimiter by additionally implementing Accept() and Wait()
  • Make client-go/tools/record use PassiveRateLimiter

Refactor EventSourceObjectSpamFilter, EventAggregator, EventCorrelator

  • EventSourceObjectSpamFilter, EventAggregator, EventCorrelator use clock.PassiveClock now.
    • This won't be a breaking change because even if a clock.Clock is passed, it still implements the clock.PassiveClock interface.
  • Extend clock.PassiveClock through Clock.
  • Replace pacakge local implementation of realClock with clock.RealClock
  • In flowcontrol/throttle.go split tokenBucketRateLimiters to use Clock and clock.PassiveClock.
  • Migrate client-go/tools/record tests from using IntervalClock to using SimpleIntervalClock (honest implementation of clock.PassiveClock)

Which issue(s) this PR fixes:

Fixes #104500

Special notes for your reviewer:

None

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/priority backlog

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. priority/backlog Higher priority than priority/awaiting-more-evidence. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 25, 2021
@MadhavJivrajani
Copy link
Contributor Author

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 25, 2021
@MadhavJivrajani
Copy link
Contributor Author

/assign @MikeSpreitzer

@MadhavJivrajani
Copy link
Contributor Author

/test pull-kubernetes-integration

@MadhavJivrajani
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-ubuntu-containerd
Due to timeout

@MikeSpreitzer
Copy link
Member

This is a good start.

We should be able to refactor further, so that a EventSourceObjectSpamFilter has a PassiveClock rather than a Clock, and NewTokenBucketPassiveRateLimiterWithClock takes a PassiveClock rather than a Clock. That would enable us to replace the use of IntervalClock with SimpleIntervalClock.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 26, 2021
@MadhavJivrajani
Copy link
Contributor Author

MadhavJivrajani commented Aug 26, 2021 •
edited

@MikeSpreitzer, in order to move from IntervalClock to SimpleIntervalClock, the clocks in events.go also needed to be changed. I went through the code and found that the only method of clock being used is Now(), which the PassiveClock provides. I've made the change of migrating that to PassiveClock as well. This won't be breaking because any clock that implements clock.Clock will also implement clock.PassiveClock.

@MadhavJivrajani MadhavJivrajani changed the title Refactor client-go/util/flowcontrol/throttle.go RateLimiter Move client-go/tools/record tests away from IntervalClock to SimpleIntervalClock Aug 26, 2021
@fedebongio
Copy link
Contributor

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 26, 2021
@MikeSpreitzer MikeSpreitzer mentioned this pull request Aug 27, 2021
Closed
MikeSpreitzer
MikeSpreitzer reviewed Aug 27, 2021
View reviewed changes
Copy link
Member

@MikeSpreitzer MikeSpreitzer left a comment

Choose a reason for hiding this comment

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

Oops, sorry, I had not noticed the Wait method of RateLimiter. That also has to be excised from PassiveRateLimiter.

staging/src/k8s.io/client-go/tools/record/event_test.go Outdated Show resolved Hide resolved
staging/src/k8s.io/client-go/util/flowcontrol/throttle.go Outdated Show resolved Hide resolved
@MadhavJivrajani MadhavJivrajani force-pushed the refactor-rate-limiters branch 3 times, most recently from 806a4a6 to 05b96de Compare September 3, 2021 07:46
@MadhavJivrajani
Copy link
Contributor Author

/test pull-kubernetes-unit

1 similar comment
@MadhavJivrajani
Copy link
Contributor Author

/test pull-kubernetes-unit

@MadhavJivrajani
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2021
This was referenced Sep 14, 2021
Closed
Merged
@MadhavJivrajani
Refactor client-go/util/flowcontrol/throttle.go RateLimiter
ac5c55f 
- Introduce PassiveRateLimiter which implements all methods of previous RateLimiter except Accept() and Wait()
- Change RateLimiter interface to extend PassiveRateLimiter by additionally implementing Accept() and Wait()
- Make client-go/tools/record use PassiveRateLimiter

Refactor EventSourceObjectSpamFilter, EventAggregator, EventCorrelator

- EventSourceObjectSpamFilter, EventAggregator, EventCorrelator use clock.PassiveClock now.
	- This won't be a breaking change because even if a clock.Clock is passed, it still implements the clock.PassiveClock interface.
- Extend clock.PassiveClock through Clock.
- Replace pacakge local implementation of realClock with clock.RealClock
- In flowcontrol/throttle.go split tokenBucketRateLimiters to use Clock and clock.PassiveClock.
- Migrate client-go/tools/record tests from using IntervalClock to using SimpleIntervalClock (honest implementation of clock.PassiveClock)

Signed-off-by: Madhav Jivrajani <madhav.jiv@gmail.com>
@MadhavJivrajani
Copy link
Contributor Author

/test all

@MadhavJivrajani
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2021
MikeSpreitzer
MikeSpreitzer approved these changes Sep 17, 2021
View reviewed changes
Copy link
Member

@MikeSpreitzer MikeSpreitzer left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2021
@MikeSpreitzer
Copy link
Member

/assign @liggitt

@MikeSpreitzer MikeSpreitzer removed their assignment Sep 17, 2021
liggitt
liggitt reviewed Sep 20, 2021
View reviewed changes
staging/src/k8s.io/client-go/util/flowcontrol/throttle.go
)

type RateLimiter interface {
type PassiveRateLimiter interface {
Copy link
Member

Choose a reason for hiding this comment

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

what's the difference between PassiveRateLimiter and RateLimiter? it's not clear why TryAccept belongs in once interface and Accept belongs in the other

Copy link
Member

Choose a reason for hiding this comment

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

PassiveRateLimiter reads time but does do any waiting.

@liggitt
Copy link
Member

liggitt commented Sep 20, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, MadhavJivrajani, MikeSpreitzer

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2021
@k8s-ci-robot k8s-ci-robot merged commit 775c931 into kubernetes:master Sep 20, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 20, 2021
@MadhavJivrajani MadhavJivrajani deleted the refactor-rate-limiters branch February 4, 2022 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

@MikeSpreitzer MikeSpreitzer MikeSpreitzer approved these changes

@RainbowMango RainbowMango Awaiting requested review from RainbowMango

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

client-go/tools/record/ tests use IntervalClock
5 participants
@MadhavJivrajani @MikeSpreitzer @fedebongio @liggitt @k8s-ci-robot