Skip to content
This repository has been archived by the owner on Dec 20, 2024. It is now read-only.

fix: unset the default value of totallimit #1121

Merged
merged 1 commit into from
Dec 5, 2019

Conversation

lowzj
Copy link
Member

@lowzj lowzj commented Dec 4, 2019

Signed-off-by: lowzj [email protected]

Ⅰ. Describe what this PR did

The total limit of dfget is used to limit downloading speed of all the tasks on the same host. It's designed to be set by user explicitly, it should not be set by default. Otherwise it doesn't work if users only set flag locallimit but not set totallimit.

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@pouchrobot pouchrobot added kind/bug This is bug report for project size/XS labels Dec 4, 2019
@starnop
Copy link
Contributor

starnop commented Dec 4, 2019

Could you please add more descriptions about why to submit this PR?

@codecov-io
Copy link

codecov-io commented Dec 4, 2019

Codecov Report

Merging #1121 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
- Coverage   47.23%   47.22%   -0.01%     
==========================================
  Files         116      116              
  Lines        7205     7204       -1     
==========================================
- Hits         3403     3402       -1     
  Misses       3535     3535              
  Partials      267      267
Impacted Files Coverage Δ
dfget/config/config.go 89.41% <ø> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e49d466...c10fde1. Read the comment docs.

@@ -96,7 +96,6 @@ func NewProperties() *Properties {
LocalLimit: DefaultLocalLimit,
MinRate: DefaultMinRate,
ClientQueueSize: DefaultClientQueueSize,
TotalLimit: DefaultTotalLimit,
Copy link
Contributor

Choose a reason for hiding this comment

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

Then could you please delete the const value DefaultTotalLimit?

@starnop
Copy link
Contributor

starnop commented Dec 4, 2019

The total limit is used to limit downloading speed of all the tasks on the same host. It's designed to be set by user explicitly, it should not be set by default. Otherwise it doesn't work if users only set flag locallimit but not set totallimit.

In fact, dfdaemon will set a default totalLimit value for dfget. Should it be deleted?

@lowzj
Copy link
Member Author

lowzj commented Dec 4, 2019

The total limit is used to limit downloading speed of all the tasks on the same host. It's designed to be set by user explicitly, it should not be set by default. Otherwise it doesn't work if users only set flag locallimit but not set totallimit.

In fact, dfdaemon will set a default totalLimit value for dfget. Should it be deleted?

It's unnecessary to delete it from dfdaemon. The dfdaemon is a specific usage scenario of dfget, it will start serval dfget processes at the same time when pulling an image, so the totallimit should be set. And the flag ratelimit of dfdaemon will set the totallimit and locallimit of dfget at the same time.

Copy link
Member

@SataQiu SataQiu left a comment

Choose a reason for hiding this comment

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

/LGTM

@starnop
Copy link
Contributor

starnop commented Dec 5, 2019

LGTM.

@starnop starnop merged commit 096afac into dragonflyoss:master Dec 5, 2019
@lowzj lowzj deleted the fix-totallimit branch December 5, 2019 02:15
inoc603 pushed a commit to inoc603/Dragonfly that referenced this pull request Dec 23, 2019
fix: unset the default value of totallimit
sungjunyoung pushed a commit to sungjunyoung/Dragonfly that referenced this pull request May 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug This is bug report for project size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants