OpenAtomFoundation / pika

Pika is a Redis-Compatible database developed by Qihoo's infrastructure team.
BSD 3-Clause "New" or "Revised" License
5.76k stars 1.19k forks source link

fix: revised TimerTaskManager and add some comments #2776

Open cheniujh opened 3 days ago

cheniujh commented 3 days ago

1 原本的TimerTaskManager正确性没有问题,但是逻辑有改进的空间,具体地:TimerTaskManager::ExecTimerTask()会返回一个timeout, 用于给epoll_wait作为timeout使用,原本返回的这个timeout值是当前定时器内所有任务中执行间隔最小的那个时间间隔的值,但实际上可以直接返回:下一个要执行的定时任务的过期时间戳 - 当前时间戳,直接让epoll等待这个差值即可,没必要找什么mininum time interval,减去这部分逻辑的同时还能降低复杂度

2 加了一些注释,如果将来要增加动态添加/删除定时任务的特性(届时如果有这个需求的话),可以按照注释的思路继续改造

3 规范了一处命名以和pika保持一致:timerTaskThread_改为了 timer_taskthread

1 The correctness of the original TimerTaskManager was not an issue, but there is room for logical improvement. Specifically, TimerTaskManager::ExecTimerTask() returns a timeout that is used as the timeout for epoll_wait. Originally, this returned timeout value was the minimum interval among all tasks in the current timer. However, it can directly return the difference between the expiration timestamp of the next scheduled task and the current timestamp. This difference can be used directly for epoll to wait, without the need to find the minimum time interval.

2 Added some comments. If there is a future requirement to add features for dynamically adding/deleting timer tasks, the modifications can be made following the approach outlined in the comments.

3 Standardized a naming convention to keep it consistent with pika: timerTaskThread_ was changed to timer_task_thread_.

coderabbitai[bot] commented 3 days ago

Walkthrough

The changes to the codebase primarily involve renaming variables and clarifying comments in the DispatchThread and TimerTaskManager classes. Specifically, the variable timerTaskThread_ has been renamed to timer_task_thread_, thread names have been adjusted, and the ExecTimerTask function's return type changed from int to int32_t. Additional comments have been included for clarity.

Changes

File Path Change Summary
src/net/src/dispatch_thread.cc Renamed timerTaskThread_ to timer_task_thread_, adjusted thread names.
src/net/src/dispatch_thread.h Renamed TimerTaskThread member variable to timer_task_thread_.
src/net/src/net_util.cc Changed return type from int to int32_t and modified logic in ExecTimerTask.
src/net/src/net_util.h Changed AddTimerTask return description; removed GetMinIntervalMs; added comments.

Poem

In the code where threads do dance,
Variables took a chance—
From timerTaskThread_ to timer_task_thread_,
The change was quite good!
With types refined and logic neat,
Now the task timings are quite fleet.
Here's to a code tune-up, a job complete! 🌟🐇


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` - `@coderabbitai help me debug CodeRabbit configuration file.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger an incremental review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai full review` to do a full review from scratch and review all the files again. - `@coderabbitai summary` to regenerate the summary of the PR. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai configuration` to show the current CodeRabbit configuration for the repository. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.
AlexStocks commented 14 hours ago

“下一个要执行的定时任务的过期时间戳 - 当前时间戳,直接让epoll等待这个差值即可,没必要找什么mininum time interval,减去这部分逻辑的同时还能降低复杂度” 关键是 epoll wait 的过期时间并不精准呀

Issues-translate-bot commented 14 hours ago

Bot detected the issue body's language is not English, translate it automatically.


"The expiration timestamp of the next scheduled task to be executed - the current timestamp, just let epoll wait for the difference. There is no need to find a minimum time interval. Subtracting this part of the logic can also reduce the complexity." The key is The expiration time of epoll wait is not accurate.

AlexStocks commented 14 hours ago

epoll_wait timeout 设置为 -1 尤为不合适,咱们这里一个网络 worker 是要处理网络和超时事件,如果只处理网络,那你设定为 -1 还是可以的

Issues-translate-bot commented 14 hours ago

Bot detected the issue body's language is not English, translate it automatically.


It is particularly inappropriate to set epoll_wait timeout to -1. We have a network worker here that handles network and timeout events. If it only handles the network, then you can set it to -1.