Skip to content

Conversation

@cameronlee314
Copy link
Contributor

Summary of API changes:

  1. TimerRegistry -> KeyScheduler; register -> schedule
  2. TimerFunction -> SchedulingFunction; registerTimer -> schedulingInit, onTimer -> executeForKey
  3. TimerCallback -> SchedulingCallback onTimer -> execute
  4. TaskContext: registerTimer -> scheduleCallback, deleteTimer -> deleteScheduledCallback

Only terminology changes are intended (e.g. classes, var names, logs). No functionality change is intended.
An upcoming PR will further update TaskContext and the access to the scheduling logic.

@cameronlee314
Copy link
Contributor Author

@xinyuiscool @prateekm can you please take a look?

@prateekm
Copy link
Contributor

@cameronlee314 Just looking at the names in the description, I'd prefer to not use key in the names since it's an overloaded term. How about something along these lines:

  1. TimerRegistry -> Scheduler; register -> schedule
  2. TimerFunction -> ScheduledFunction; registerTimer -> schedule, onTimer -> onCallback
  3. TimerCallback -> ScheduledCallback onTimer -> onCallback
  4. TaskContext: registerTimer -> scheduleCallback, deleteTimer -> deleteScheduledCallback

This way we're only introducing 2 new terms - schedule and callback, and both are fairly familiar concepts.

@cameronlee314
Copy link
Contributor Author

Thanks for the suggestion @prateekm. I wasn't too fond of KeyScheduler either. The reason I did use that name was that when we consolidate TaskContext.scheduleCallback and TaskContext.deleteScheduledCallback into a TaskContext.something, I was thinking of using "Scheduler" for that. Now, I'm thinking that I could use something like SchedulerCallbackRegistry for that TaskContext case.

@cameronlee314
Copy link
Contributor Author

@xinyuiscool @prateekm could you please take another look at this?

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for cleaning this up. Some documentation related suggestions.

Copy link
Contributor Author

@cameronlee314 cameronlee314 left a comment

Choose a reason for hiding this comment

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

Thanks for the doc suggestions. I like them much better than what I came up with.

@cameronlee314
Copy link
Contributor Author

@prateekm @xinyuiscool could you please take another look?

Copy link
Contributor

@prateekm prateekm left a comment

Choose a reason for hiding this comment

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

Thanks!

@asfgit asfgit closed this in 10607f0 Sep 26, 2018
@cameronlee314 cameronlee314 deleted the rename_timer branch October 4, 2019 21:54
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.

2 participants