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

refactor: refactor the old goroutine execution sweep #18361

Merged
merged 1 commit into from
Mar 21, 2023
Merged

refactor: refactor the old goroutine execution sweep #18361

merged 1 commit into from
Mar 21, 2023

Conversation

chlins
Copy link
Member

@chlins chlins commented Mar 16, 2023

  1. Delete the old goroutine execution sweeper when create execution(in the case of high concurrency can cause goroutine backlogs, affect the performance of core)
  2. Introduce the new way to sweep executions, a global scheduled job will take the work.

Thank you for contributing to Harbor!

Comprehensive Summary of your change

Issue being fixed

Fixes #(issue)

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

@chlins chlins requested a review from a team as a code owner March 16, 2023 04:48
@codecov
Copy link

codecov bot commented Mar 16, 2023 •
edited
Loading

Codecov Report

Merging #18361 (d1eb8ca) into main (2b3f178) will increase coverage by 3.13%.
The diff coverage is 37.58%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #18361      +/-   ##
==========================================
+ Coverage   67.41%   70.55%   +3.13%     
==========================================
  Files         976      747     -229     
  Lines      106493    93634   -12859     
  Branches     2662        0    -2662     
==========================================
- Hits        71794    66061    -5733     
+ Misses      30840    24004    -6836     
+ Partials     3859     3569     -290
Flag Coverage Δ
unittests 70.55% <37.58%> (+3.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/controller/gc/controller.go 72.26% <ø> (-0.60%) ⬇️
src/controller/p2p/preheat/enforcer.go 54.84% <ø> (-0.38%) ⬇️
src/controller/replication/execution.go 69.41% <ø> (-0.54%) ⬇️
src/controller/retention/controller.go 47.63% <ø> (-0.67%) ⬇️
src/controller/scan/callback.go 82.35% <0.00%> (ø)
src/controller/scandataexport/execution.go 87.83% <ø> (-0.17%) ⬇️
src/controller/systemartifact/execution.go 72.52% <ø> (-0.60%) ⬇️
src/jobservice/job/known_jobs.go 0.00% <0.00%> (ø)
src/pkg/task/execution.go 67.43% <ø> (+5.97%) ⬆️
src/controller/task/sweep.go 10.76% <10.76%> (ø)
... and 6 more

... and 238 files with indirect coverage changes

@chlins chlins assigned wy65701436 and unassigned zyyw and MinerYang Mar 16, 2023
@chlins chlins added target/2.8.0 release-note/update Update or Fix kind/refactor The issue is to track the effort of refactory. labels Mar 16, 2023
@chlins chlins force-pushed the refactor/execution-sweep branch 4 times, most recently from 010d953 to 6931144 Compare March 16, 2023 09:21
stonezdj
stonezdj reviewed Mar 19, 2023
View reviewed changes
src/pkg/task/sweep_job.go Outdated Show resolved Hide resolved
stonezdj
stonezdj approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@stonezdj stonezdj left a comment

Choose a reason for hiding this comment

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

LGTM

@chlins chlins force-pushed the refactor/execution-sweep branch 2 times, most recently from 5cfaa59 to 9334456 Compare March 20, 2023 06:31
wy65701436
wy65701436 approved these changes Mar 20, 2023
View reviewed changes
Copy link
Contributor

@wy65701436 wy65701436 left a comment

Choose a reason for hiding this comment

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

lgtm

@chlins
refactor: refactor the old goroutine execution sweep to global execut…
d1eb8ca 
…ion sweep job

1. Delete the old goroutine execution sweeper when create execution.(in the case of high concurrency can cause goroutine backlogs, affect the performance of core)
2. Introduce the new way to sweep executions, a global scheduled job will take the work.

Signed-off-by: chlins <[email protected]>
@chlins chlins merged commit f21b148 into goharbor:main Mar 21, 2023
@chlins chlins mentioned this pull request Mar 22, 2023
Closed
@Vad1mo Vad1mo added the area/gc label Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wy65701436 wy65701436 wy65701436 approved these changes

@stonezdj stonezdj stonezdj approved these changes

Assignees

@stonezdj stonezdj

@wy65701436 wy65701436

Labels
area/gc kind/refactor The issue is to track the effort of refactory. release-note/update Update or Fix target/2.8.0
Projects
GC Improvement Activities
Status: Completed
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chlins @stonezdj @wy65701436 @Vad1mo @zyyw @MinerYang