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

fix #8599: move task schedule to a separate goroutine #9775

Merged
merged 7 commits into from Jun 9, 2023

Conversation

gouhongshen
Copy link
Contributor

@gouhongshen gouhongshen commented May 31, 2023

What type of PR is this?

  • API-change
  • [*] BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #8599

What this PR does / why we need it:

moving the task schedule from the ticker normal routine to a separate goroutine can avoid the hakeeper's health check and tick update operations being blocked by the task schedule, or the tick will be skipped and can not correctly estimate the time passing.

and I have already added some unit tests.

@CLAassistant
Copy link

CLAassistant commented May 31, 2023

CLA assistant check
All committers have signed the CLA.

@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label May 31, 2023
@matrix-meow
Copy link
Contributor

@gouhongshen Thanks for your contributions!

The pull request aims to fix issue #8599 by moving the task schedule from the ticker normal routine to a separate goroutine. The changes include adding a new function tickerForTaskSchedule to handle task scheduling, modifying the ticker function to call tickerForTaskSchedule in a separate goroutine, and modifying the hakeeperCheck function to call getCheckerStateFromLeader instead of taskSchedule. The changes also include adding unit tests for the new function and for getCheckerStateFromLeader.

There are no major issues with the changes made in the pull request. However, there are a few minor issues that need to be addressed:

  1. The title of the pull request is not very descriptive. It would be better to include more information about what the pull request does, such as "Move task schedule to separate goroutine to avoid blocking hakeeper operations".

  2. The body of the pull request is brief and lacks detail. It would be helpful to include more information about why the changes were made and how they improve the codebase.

  3. The changes made in the pull request are not optimized. The tickerForTaskSchedule function could be simplified by removing the second select statement and using a single select statement with a default case. This would make the code easier to read and understand.

To address these issues, the pull request title and body should be updated to provide more detail about the changes made and their benefits. The tickerForTaskSchedule function should be simplified to improve readability.

@gouhongshen gouhongshen added the kind/bug Something isn't working label May 31, 2023
@mergify mergify bot merged commit e9baca1 into matrixorigin:main Jun 9, 2023
5 checks passed
sukki37 added a commit that referenced this pull request Jun 10, 2023
sukki37 added a commit that referenced this pull request Jun 10, 2023
@zhangxu19830126
Copy link
Contributor

@gouhongshen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/M Denotes a PR that changes [100,499] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants