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

timer: add user interfaces for timer framework #43837

Merged
merged 1 commit into from
May 17, 2023

Conversation

lcwangchao
Copy link
Collaborator

What problem does this PR solve?

Issue Number: close #43836

What is changed and how it works?

  • Add a new package timer for timer framework
  • Add some new interfaces for user
    • Introduce the model TimerRecord to present the meta of the timer
    • Introduce TimerClient to provide some operations of timer for user
    • Introduce Hook for the users to custom actions when timer event is triggered

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot
Copy link

ti-chi-bot bot commented May 15, 2023

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • YangKeao
  • hawkingrei

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot bot added release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 15, 2023
@lcwangchao lcwangchao force-pushed the timer_user_interface branch 3 times, most recently from 2d7a267 to 3cb5c24 Compare May 16, 2023 03:14
@lcwangchao
Copy link
Collaborator Author

/retest

@ti-chi-bot ti-chi-bot bot added the status/LGT1 Indicates that a PR has LGTM 1. label May 16, 2023
@hawkingrei
Copy link
Member

/review default

@ti-chi-bot
Copy link

ti-chi-bot bot commented May 16, 2023

@hawkingrei:

Summary

  • PR Title: timer: add user interfaces for timer framework
  • Issue Number: Close #43836
  • Diff Size: +2387 -1

The PR introduces a new package timer along with user interfaces to work with timer framework. The following changes are introduced:

  1. A new package timer is added for the timer framework.
  2. New interfaces for users are created including TimerRecord, TimerClient, and Hook.
  3. Unit tests are added for the new package and its features.
  4. Changes are made to build/nogo_config.json to include timer code.
  5. A new file timer/api/BUILD.bazel has been added.

Potential problems

No potential issues are identified.

Suggestions

It is suggested to add further details in the PR description regarding:

  1. The context behind the timer framework, its purpose and the problem it'll resolve.
  2. Any changes or refactoring in existing codebase related to the new package and user interfaces.
  3. Any future plans related to the project.

In response to this:

/review default

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/test-infra repository.

@lcwangchao lcwangchao force-pushed the timer_user_interface branch 2 times, most recently from 11f1edd to 6dfd90c Compare May 16, 2023 06:37
@lcwangchao
Copy link
Collaborator Author

/retest

@lcwangchao lcwangchao force-pushed the timer_user_interface branch 2 times, most recently from ffca71c to edadc41 Compare May 16, 2023 11:27
Copy link
Member

@YangKeao YangKeao left a comment

Choose a reason for hiding this comment

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

Good job. Rest LGTM

timer/api/client.go Outdated Show resolved Hide resolved
timer/api/timer.go Show resolved Hide resolved
}

// SchedIntervalPolicy implements SchedEventPolicy, it is the policy of type `SchedEventInterval`
type SchedIntervalPolicy struct {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder what's the expected behavior if the client found it's too late to start the job (e.g. if the cluster is down for one or two days.). If they are only expected to execute when the time is within a small offset around the "expected" schedule (e.g. */5 * * * * shouldn't happen at 00:02), a function like "NearestSchedTime" (or other better names) which will return the nearest "expected" schedule time would be really helpful.

The only difference between NearestSchedTime and for-loop with NextEventTime is that the NearestSchedTime can calculate much faster if the timer is regular (e.g. for every 5 seconds). If we don't have this, we'll need to introduce a new range (or limitation) to avoid the "for-loop next" calculates forever, which will also make the expected behavior hard to describe/learn (See the .spec.startingDeadlineSeconds in kubernetes cronjob as a counter example 🤦‍♂️ )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch! I think there is a principle in the framework that there is only one event triggering at the same time. For example, if the timer is set to trigger at the first minute of each hour but the cluster is down for one or two days. When the cluster recovered, there was still only one event triggered even if we have many events after the watermark.

Whether to execute all these jobs or ignore some "expired" depends on the user:

  1. If the user wants to execute all passed jobs, he should compute NextEventTime again and again until the returned event time after now.
  2. If the user wants to ignore some jobs after a certain time, he can pass that time instead of the watermark to NextEventTime to compute the next event time after it.

When processing one event, a user can submit one or more jobs concurrency, it dependens to him.

@ti-chi-bot ti-chi-bot bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 17, 2023
@lcwangchao
Copy link
Collaborator Author

/merge

@ti-chi-bot
Copy link

ti-chi-bot bot commented May 17, 2023

This pull request has been accepted and is ready to merge.

Commit hash: 89215c5

@ti-chi-bot ti-chi-bot bot added the status/can-merge Indicates a PR has been approved by a committer. label May 17, 2023
@lcwangchao
Copy link
Collaborator Author

/retest

@ti-chi-bot ti-chi-bot bot merged commit b22d23e into pingcap:master May 17, 2023
8 checks passed
@lcwangchao lcwangchao deleted the timer_user_interface branch May 17, 2023 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

define user models/interfaces of timer framework
3 participants