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

feat: add traffic type of peer task download duration #2349

Merged
merged 1 commit into from
May 16, 2023

Conversation

litianxi
Copy link
Contributor

feat: add traffic type of peer task download duration

@litianxi litianxi requested a review from a team as a code owner May 13, 2023 02:19
@codecov
Copy link

codecov bot commented May 13, 2023

Codecov Report

Merging #2349 (7b8226e) into main (cd0b95d) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2349      +/-   ##
==========================================
+ Coverage   49.71%   49.76%   +0.05%     
==========================================
  Files         148      148              
  Lines       19640    19644       +4     
==========================================
+ Hits         9764     9776      +12     
+ Misses       9265     9255      -10     
- Partials      611      613       +2     
Flag Coverage Δ
Object-compatibility-e2etests ?
e2etests ?
unittests 49.76% <100.00%> (+0.05%) ⬆️

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

Impacted Files Coverage Δ
scheduler/metrics/metrics.go 100.00% <ø> (ø)
scheduler/service/service_v1.go 79.38% <100.00%> (+0.03%) ⬆️
scheduler/service/service_v2.go 83.78% <100.00%> (+0.03%) ⬆️

... and 3 files with indirect coverage changes

Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

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

Please remove the changes in the scheduler/service/service_v2.go. I will finish it.

scheduler/service/service_v1.go Outdated Show resolved Hide resolved
@litianxi litianxi force-pushed the main branch 2 times, most recently from f98e9c0 to d579a51 Compare May 15, 2023 04:59
Copy link
Member

@gaius-qi gaius-qi 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

@gaius-qi gaius-qi 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

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

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

Fix unit testing.

2023-05-15T07:22:43.5857994Z --- FAIL: TestServiceV2_handleDownloadPeerFinishedRequest (0.00s)
2023-05-15T07:22:43.5858403Z     --- PASS: TestServiceV2_handleDownloadPeerFinishedRequest/peer_can_not_be_loaded (0.00s)
2023-05-15T07:22:43.5858899Z     --- PASS: TestServiceV2_handleDownloadPeerFinishedRequest/peer_state_is_PeerStateSucceeded (0.00s)
2023-05-15T07:22:43.5859335Z     --- FAIL: TestServiceV2_handleDownloadPeerFinishedRequest/peer_state_is_PeerStateRunning (0.00s)
2023-05-15T07:22:43.5859633Z panic: inconsistent label cardinality: expected 6 label values but got 5 in []string{"LEVEL0", "DFDAEMON", "d7y", "foo", "normal"} [recovered]
2023-05-15T07:22:43.5859958Z 	panic: inconsistent label cardinality: expected 6 label values but got 5 in []string{"LEVEL0", "DFDAEMON", "d7y", "foo", "normal"} [recovered]
2023-05-15T07:22:43.5860256Z 	panic: inconsistent label cardinality: expected 6 label values but got 5 in []string{"LEVEL0", "DFDAEMON", "d7y", "foo", "normal"}

@litianxi
Copy link
Contributor Author

Fix unit testing.

2023-05-15T07:22:43.5857994Z --- FAIL: TestServiceV2_handleDownloadPeerFinishedRequest (0.00s)
2023-05-15T07:22:43.5858403Z     --- PASS: TestServiceV2_handleDownloadPeerFinishedRequest/peer_can_not_be_loaded (0.00s)
2023-05-15T07:22:43.5858899Z     --- PASS: TestServiceV2_handleDownloadPeerFinishedRequest/peer_state_is_PeerStateSucceeded (0.00s)
2023-05-15T07:22:43.5859335Z     --- FAIL: TestServiceV2_handleDownloadPeerFinishedRequest/peer_state_is_PeerStateRunning (0.00s)
2023-05-15T07:22:43.5859633Z panic: inconsistent label cardinality: expected 6 label values but got 5 in []string{"LEVEL0", "DFDAEMON", "d7y", "foo", "normal"} [recovered]
2023-05-15T07:22:43.5859958Z 	panic: inconsistent label cardinality: expected 6 label values but got 5 in []string{"LEVEL0", "DFDAEMON", "d7y", "foo", "normal"} [recovered]
2023-05-15T07:22:43.5860256Z 	panic: inconsistent label cardinality: expected 6 label values but got 5 in []string{"LEVEL0", "DFDAEMON", "d7y", "foo", "normal"}

It failed the service v2's unit test, it seems that if the service v2's logic is not modified, it will be affected

Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

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

LGTM

@gaius-qi gaius-qi merged commit acff931 into dragonflyoss:main May 16, 2023
gaius-qi pushed a commit that referenced this pull request Jun 28, 2023
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