bugfix: modify two supernode api's bug #682
Conversation
|
Please carefully check the CI failures. Thanks a lot. @yunfeiyangbuaa |
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
from
50646bb to
bab1b04
Compare
Done |
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
- Coverage 46.86% 46.74% -0.13%
==========================================
Files 108 108
Lines 6226 6240 +14
==========================================
- Hits 2918 2917 -1
- Misses 3075 3085 +10
- Partials 233 238 +5
Continue to review full report at Codecov.
|
4 similar comments
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
- Coverage 46.86% 46.74% -0.13%
==========================================
Files 108 108
Lines 6226 6240 +14
==========================================
- Hits 2918 2917 -1
- Misses 3075 3085 +10
- Partials 233 238 +5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
- Coverage 46.86% 46.74% -0.13%
==========================================
Files 108 108
Lines 6226 6240 +14
==========================================
- Hits 2918 2917 -1
- Misses 3075 3085 +10
- Partials 233 238 +5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
- Coverage 46.86% 46.74% -0.13%
==========================================
Files 108 108
Lines 6226 6240 +14
==========================================
- Hits 2918 2917 -1
- Misses 3075 3085 +10
- Partials 233 238 +5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
- Coverage 46.86% 46.74% -0.13%
==========================================
Files 108 108
Lines 6226 6240 +14
==========================================
- Hits 2918 2917 -1
- Misses 3075 3085 +10
- Partials 233 238 +5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
- Coverage 47.39% 47.28% -0.11%
==========================================
Files 109 109
Lines 6294 6306 +12
==========================================
- Hits 2983 2982 -1
- Misses 3078 3086 +8
- Partials 233 238 +5
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #682 +/- ##
==========================================
- Coverage 46.86% 46.74% -0.13%
==========================================
Files 108 108
Lines 6226 6240 +14
==========================================
- Hits 2918 2917 -1
- Misses 3075 3085 +10
- Partials 233 238 +5
Continue to review full report at Codecov.
|
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
2 times, most recently
from
b013df3 to
200ed54
Compare
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
2 times, most recently
from
dfc9b7e to
b4ecc66
Compare
There was a problem hiding this comment.
@starnop @allencloud I have modified the code according to your advice
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
from
b4ecc66 to
8729dc3
Compare
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
from
8729dc3 to
110ac03
Compare
yunfeiyanggzq
changed the title
allencloud
changed the title
| @@ -90,7 +93,9 @@ func (api *supernodeAPI) PullPieceTask(node string, req *types.PullPieceTaskRequ | |||
| api.Scheme, node, peerPullPieceTaskPath, util.ParseQuery(req)) | |||
|
|
|||
| resp = new(types.PullPieceTaskResponse) | |||
| e = api.get(url, resp) | |||
| if e = api.get(url, resp); e != nil { | |||
There was a problem hiding this comment.
Why to change this? I do not see any difference after the change actually.
There was a problem hiding this comment.
There was a problem hiding this comment.
if the supernode is off,the resp={} in the previous code.and res.code will make the dfget happen a a invalid memory address or nil pointer dereference err
There was a problem hiding this comment.
I return a nil if the supernode if off so the the program will go on
There was a problem hiding this comment.
It is so tricky. I would like to see you change the function definition. Please do not define the variables in the function declaration. Just define them in the implementation block.
There was a problem hiding this comment.
It is so tricky. I would like to see you change the function definition. Please do not define the variables in the function declaration. Just define them in the implementation block.
In fact I can't understand your comment very clearly,Can in more details?.But I think the coder want to return a nil when a err happen such as.Dragonfly/dfget/core/api/supernode_api.go
Line 73 in c5d0873
and he just ignore this place in the previous implement .
dfget/core/api/supernode_api.go
Outdated
| resp = new(types.BaseResponse) | ||
| e = api.get(url, resp) | ||
| if e != nil { | ||
| logrus.Infof("failed to report piece{taskid:%s,range:%s},err: %v", req.TaskID, req.PieceRange, e) |
There was a problem hiding this comment.
Please confirm that here is an Infof level log, or Errorf level log?
There was a problem hiding this comment.
@starnop #682 (comment)
Error or Info ?
There was a problem hiding this comment.
I am sure that Info level is improper. Both Error and Warn are better.
There was a problem hiding this comment.
I am sure that
Infolevel is improper. BothErrorandWarnare better.
I am agree with you.I will modify it
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
from
110ac03 to
9f3bddf
Compare
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
from
9f3bddf to
9632aaa
Compare
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
from
9632aaa to
5276082
Compare
done |
|
LGTM. @yunfeiyangbuaa And could you please make a rebase? |
Signed-off-by: yunfeiyangbuaa <[email protected]>
yunfeiyanggzq
force-pushed
the
supernodeAPI
branch
from
5276082 to
cbe57dc
Compare
Done |
| resp = new(types.BaseResponse) | ||
| e = api.get(url, resp) | ||
| if e = api.get(url, resp); e != nil { | ||
| logrus.Errorf("failed to report piece{taskid:%s,range:%s},err: %v", req.TaskID, req.PieceRange, e) |
There was a problem hiding this comment.
Just return error in API implementation rather than logging them. Then the caller can choose to log the error themselves.
There was a problem hiding this comment.
dfgte haven't check all the err and response.so should I and a check ? @lowzj
There was a problem hiding this comment.
#682 (comment)
I added a check before. and I think it is a good idea to add checks in supernode api
|
LGTM. |
bugfix: modify two supernode api's bug
bugfix: modify two supernode api's bug
Signed-off-by: yunfeiyangbuaa [email protected]
Ⅰ. Describe what this PR did
json.Unmarshal(body, resp),it will produce anerr unexpected end of JSON inputerr.I add a status code info to the supernode's response to resolve these bugs and do with the err and status code{}instead ofnil,the coder usenilto check the res,so it will happena invalid memory address or nil pointer dereferenceerr .i make the return be nil when the supernode is offⅡ. Does this pull request fix one issue?
fixes #680
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Ⅳ. Describe how to verify it
you can add a fmt.println() to the previous code's api return err,it will report an err.and my code won't
Ⅴ. Special notes for reviews