curvefs/client: limited download retry times #2130
Conversation
|
recheck |
1 similar comment
|
recheck |
curvefs/src/client/fuse_s3_client.h
Outdated
| @@ -34,6 +34,8 @@ | |||
| #include "curvefs/src/client/s3/client_s3_cache_manager.h" | |||
| #include "src/common/s3_adapter.h" | |||
|
|
|||
| const int MAX_RETRY_TIME = 3; | |||
There was a problem hiding this comment.
it's better to read the max retry time from conf file
| @@ -277,7 +277,18 @@ void FuseS3Client::WarmUpAllObjs( | |||
| delete []context->buf; | |||
| return; | |||
| } | |||
| // todo: retry | |||
| if (context->retry >= MAX_RETRY_TIME) { | |||
There was a problem hiding this comment.
context->retry not initialized
| LOG(WARNING) << "Up to max retry times"; | ||
| } | ||
| delete []context->buf; | ||
| return; |
There was a problem hiding this comment.
WarmUpAllObjs may be failed, should add return status at WarmUpAllObjs
It is necessary to judge the return value of WarmUpAllObjs at the place where WarmUpAllObjs is called
There was a problem hiding this comment.
WarmUpAllObjs may be failed, should add return status at WarmUpAllObjs
It is necessary to judge the return value of WarmUpAllObjs at the place where WarmUpAllObjs is called
there is no need to do that, because Warm-up is just a matter of downloading as much as possible, not a necessity
|
recheck |
3 similar comments
|
recheck |
|
recheck |
|
recheck |
curvefs/src/client/fuse_s3_client.h
Outdated
| @@ -121,6 +123,7 @@ class FuseS3Client : public FuseClient { | |||
| Thread bgFetchThread_; | |||
| std::atomic<bool> bgFetchStop_; | |||
| std::mutex fetchMtx_; | |||
| uint32_t downloadMaxRetryTimes; | |||
There was a problem hiding this comment.
downloadMaxRetryTimes_ is better.
| delete []context->buf; | ||
| return; | ||
| } | ||
| context->retry++; |
There was a problem hiding this comment.
maybe
if ( ++context->retry >= downloadMaxRetryTimes) { is better
There was a problem hiding this comment.
I accept this suggistion, but i still think the original type is much easier to understand
| VLOG(6) << "pendingReq is over"; | ||
| cond.Signal(); | ||
| } else { | ||
| LOG(WARNING) << "Up to max retry times"; |
There was a problem hiding this comment.
no need else？ because we always need this output. and please add the objname that is download failed
Signed-off-by: CodeFarmerPK CodeFarmer.PK@Gmail.com
What problem does this PR solve?
Issue Number: #xxx
Problem Summary:
What is changed and how it works?
What's Changed:
How it Works:
Side effects(Breaking backward compatibility? Performance regression?):
Check List