Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Normalize the job queue latency's value #21355

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stonezdj
Copy link
Contributor

fix #21354

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.

  fix goharbor#21354

Signed-off-by: stonezdj <stone.zhang@broadcom.com>
Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.27%. Comparing base (c8c11b4) to head (77756a4).
Report is 341 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #21355      +/-   ##
==========================================
+ Coverage   45.36%   46.27%   +0.91%     
==========================================
  Files         244      247       +3     
  Lines       13333    13883     +550     
  Branches     2719     2875     +156     
==========================================
+ Hits         6049     6425     +376     
- Misses       6983     7121     +138     
- Partials      301      337      +36     
Flag Coverage Δ
unittests 46.27% <ø> (+0.91%) ⬆️

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

see 491 files with indirect coverage changes

Copy link
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@bupd bupd left a comment

Choose a reason for hiding this comment

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

LGTM!

@stonezdj stonezdj marked this pull request as draft December 26, 2024 06:04
@@ -267,10 +267,15 @@ func (w *monitorController) ListQueues(ctx context.Context) ([]*jm.Queue, error)
if skippedUnusedJobType(queue.JobName) {
continue
}
// normalize latency, it sometimes return -1
var latency int64
if queue.Latency > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pause this PR for further discussion.

Personally, I don't think changing the negative number to 0 is the right approach. The latency should always be positive, and a negative value typically indicates a misconfiguration, such as when the system time on the worker node running harbor-core and harbor-jobservice are not synchronized with the same time server.

Adjusting the value to 0 might mask the underlying issue for the administrator. I prefer that we add a debug log to help identify the potential cause of this situation.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
release-note/update Update or Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Normalize the latency in the job queue status
6 participants