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

Conversation

@lowzj
Copy link
Member

@lowzj lowzj commented Apr 27, 2020

Signed-off-by: lowzj [email protected]

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes #1307

should be merged into branch: master, 1.0.x

Ⅲ. 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/M labels Apr 27, 2020
@pouchrobot pouchrobot added size/M and removed size/M labels Apr 28, 2020
p2p.maxTimeout *= 2
}
actual, expected := p2p.sleepInterval()
logrus.Infof("pull piece task(%+v) result:%s and sleep actual:%.3fs expected:%.3fs",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to record expected sleep interval which is actually a random value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's more like a debug log, but I think it's necessary to output it in info level if actual > expected.


// NewClientStreamWriter creates and initialize a ClientStreamWriter instance.
func NewClientStreamWriter(clientQueue queue.Queue, api api.SupernodeAPI, cfg *config.Config) *ClientStreamWriter {
func NewClientStreamWriter(clientQueue, notifyQueue queue.Queue, api api.SupernodeAPI, cfg *config.Config) *ClientStreamWriter {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should abstract an interface for client writer, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

agreed, there is an ambitious plan to do this, but not in this pr.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@starnop
Copy link
Contributor

starnop commented Apr 28, 2020

LGTM.

@starnop
Copy link
Contributor

starnop commented Apr 28, 2020

Well, I think I understand what you want to do, and LGTM for this PR.
However, it is difficult to understand simply with code only, I think some comments should be added for the sleepInterval function and notifyQueue field. 😄

@lowzj
Copy link
Member Author

lowzj commented Apr 28, 2020

Well, I think I understand what you want to do, and LGTM for this PR.
However, it is difficult to understand simply with code only, I think some comments should be added for the sleepInterval function and notifyQueue field. 😄

DONE

@starnop starnop merged commit 77c995d into dragonflyoss:master Apr 29, 2020
@lowzj lowzj deleted the fix-sleep branch May 3, 2020 06:19
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/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: dfget sleep much time to wait for pulling next downloading piece

3 participants