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

bugfix: retry multi times if failed to report pieces #1185

Merged
merged 1 commit into from
Jan 16, 2020

Conversation

starnop
Copy link
Contributor

@starnop starnop commented Jan 15, 2020

Signed-off-by: Starnop [email protected]

Ⅰ. Describe what this PR did

Sometimes dfget will fail to report pieces to supernode for some reason, such as supernode cannot handle it. And then supernode will mark that this task has not been completed forever. That's not expected. So we should add a retry mechanism to reduce the chance of errors.

Ⅱ. 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/S labels Jan 15, 2020
@starnop starnop force-pushed the retry-report-piece branch 2 times, most recently from f9b1784 to 8075038 Compare January 16, 2020 01:47
@codecov-io
Copy link

codecov-io commented Jan 16, 2020

Codecov Report

Merging #1185 into master will decrease coverage by 0.12%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1185      +/-   ##
==========================================
- Coverage   47.66%   47.54%   -0.13%     
==========================================
  Files         115      115              
  Lines        7225     7240      +15     
==========================================
- Hits         3444     3442       -2     
- Misses       3508     3524      +16     
- Partials      273      274       +1
Impacted Files Coverage Δ
...et/core/downloader/p2p_downloader/client_writer.go 5.66% <0%> (-0.87%) ⬇️
dfget/core/api/supernode_api.go 65.85% <0%> (-3.29%) ⬇️

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 72929df...5d248de. Read the comment docs.

}

var retry = 0
var maxRetryTime = 3
Copy link
Member

Choose a reason for hiding this comment

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

The following codes will send request to supernode at most 4 times, is it expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

break
}

sleepTime := time.Duration(rand.Intn(1000)+500) * time.Millisecond
Copy link
Member

Choose a reason for hiding this comment

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

The time to sleep is a bit long. It's better to decrease the interval time of first retry. How about [50, 500]ms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE.

@starnop starnop force-pushed the retry-report-piece branch from 8075038 to b7ec36e Compare January 16, 2020 02:34
@starnop starnop force-pushed the retry-report-piece branch from b7ec36e to 5d248de Compare January 16, 2020 02:38
Copy link
Member

@lowzj lowzj left a comment

Choose a reason for hiding this comment

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

LGTM

@lowzj lowzj merged commit e7332a3 into dragonflyoss:master Jan 16, 2020
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/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants