Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

feature: support dfget params for each proxy #826

Closed

Conversation

yeya24
Copy link
Collaborator

@yeya24 yeya24 commented Aug 15, 2019

Ⅰ. Describe what this PR did

Add support for each proxy to use different dfget params.

Ⅱ. Does this pull request fix one issue?

NONE

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

@yeya24 yeya24 force-pushed the feature/add-dfgetconfig-each-proxy branch 2 times, most recently from 73be19f to 5cdbb7b Compare August 15, 2019 16:12
@codecov-io
Copy link

codecov-io commented Aug 15, 2019

Codecov Report

Merging #826 into master will increase coverage by 0.27%.
The diff coverage is 41.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #826      +/-   ##
==========================================
+ Coverage   39.67%   39.95%   +0.27%     
==========================================
  Files         109      109              
  Lines        6432     6367      -65     
==========================================
- Hits         2552     2544       -8     
+ Misses       3672     3615      -57     
  Partials      208      208
Impacted Files Coverage Δ
dfdaemon/config/config.go 88.09% <0%> (ø) ⬆️
dfdaemon/proxy/downloader.go 0% <0%> (ø)
dfdaemon/proxy/proxy.go 8.08% <76.92%> (-0.47%) ⬇️
supernode/util/locker.go 0% <0%> (-10%) ⬇️
supernode/daemon/util/store.go 91.3% <0%> (-8.7%) ⬇️
hack/boilerplate/check-boilerplate.go 33.33% <0%> (-3.26%) ⬇️
supernode/config/config.go 63.41% <0%> (-3.26%) ⬇️
supernode/daemon/mgr/progress/state_sync_map.go 4.16% <0%> (-1.39%) ⬇️
supernode/daemon/mgr/progress/progress_manager.go 23.84% <0%> (-1.16%) ⬇️
supernode/daemon/mgr/scheduler/manager.go 24.06% <0%> (-0.76%) ⬇️
... and 19 more

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 219498a...e7c0128. Read the comment docs.

@yeya24 yeya24 force-pushed the feature/add-dfgetconfig-each-proxy branch from 5cdbb7b to c8b6bec Compare August 16, 2019 03:47
@starnop starnop requested review from inoc603 and lowzj August 24, 2019 13:54
@yeya24 yeya24 force-pushed the feature/add-dfgetconfig-each-proxy branch from c8b6bec to de0ee15 Compare August 25, 2019 15:34
@yeya24 yeya24 force-pushed the feature/add-dfgetconfig-each-proxy branch from de0ee15 to e7c0128 Compare August 25, 2019 15:55
@inoc603
Copy link
Member

inoc603 commented Aug 26, 2019

The idea is good and the code looks fine.

But I'm a little concerned about the overriding dfget params part. Before we actually use a struct for dfget config as proposed in #824 , appending custom parameters to global parameters does not feel very robust. I'm sure it will work, but I think we should write tests to verify the override behavior. Also we should check if we print the final args which might have duplicated parameters in them.

@yeya24
Copy link
Collaborator Author

yeya24 commented Aug 28, 2019

The idea is good and the code looks fine.

But I'm a little concerned about the overriding dfget params part. Before we actually use a struct for dfget config as proposed in #824 , appending custom parameters to global parameters does not feel very robust. I'm sure it will work, but I think we should write tests to verify the override behavior. Also we should check if we print the final args which might have duplicated parameters in them.

Totally agree with you.
I think your question about this dfget params behavior is from the slice struct of params. I think finally we will use a struct to represent params, instead of just slices.
Let @starnop give his idea.

@starnop
Copy link
Contributor

starnop commented Aug 30, 2019

Yeah, I agree with you. @yeya24 @inoc603

And I think we can do as @inoc603 said here firstly.

@yeya24
Copy link
Collaborator Author

yeya24 commented Sep 18, 2019

So what's the plan of this pr? Do I need to work on this again? Do we plan to rework the dfget configuration as my previous pr did?

@yeya24
Copy link
Collaborator Author

yeya24 commented Jun 30, 2020

@starnop Hello, do you think this feature still needed? If yes, I can do a rebase.

@pouchrobot
Copy link
Collaborator

ping @yeya24
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

@yeya24
Copy link
Collaborator Author

yeya24 commented Jul 17, 2020

Close this pr since it seems not needed anymore.

@yeya24 yeya24 closed this Jul 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants