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

refactor: use package server/api to register supernode's API #1366

Merged
merged 1 commit into from
Jul 2, 2020

Conversation

lowzj
Copy link
Member

@lowzj lowzj commented May 28, 2020

Signed-off-by: lowzj zj3142063@gmail.com

Ⅰ. Describe what this PR did

use package server/api to register supernode's APIs

Ⅱ. 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

@lowzj lowzj force-pushed the refactor-api-register branch from 3e559bd to 69f815a Compare May 28, 2020 11:43
@lowzj lowzj force-pushed the refactor-api-register branch from 69f815a to f0671e5 Compare May 28, 2020 11:51
@codecov-commenter
Copy link

codecov-commenter commented May 28, 2020

Codecov Report

Merging #1366 into master will increase coverage by 0.24%.
The diff coverage is 69.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1366      +/-   ##
==========================================
+ Coverage   52.02%   52.26%   +0.24%     
==========================================
  Files         137      136       -1     
  Lines        9108     9104       -4     
==========================================
+ Hits         4738     4758      +20     
+ Misses       3987     3961      -26     
- Partials      383      385       +2     
Impacted Files Coverage Δ
supernode/server/server.go 43.07% <0.00%> (ø)
supernode/server/preheat_bridge.go 15.00% <18.18%> (-6.34%) ⬇️
supernode/server/api/utils.go 72.58% <50.00%> (-27.42%) ⬇️
supernode/server/api/api.go 85.18% <63.63%> (-14.82%) ⬇️
supernode/server/router.go 94.02% <94.64%> (+26.81%) ⬆️
supernode/daemon/mgr/scheduler/manager.go 21.91% <0.00%> (-0.69%) ⬇️

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 f372031...63832e8. Read the comment docs.

//
// TODO:
// Should the response body should be empty if the data is nil or empty
// string? Now it's incompatible with the client.
Copy link
Contributor

@zhouhaibing089 zhouhaibing089 May 31, 2020

Choose a reason for hiding this comment

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

I don't think we should. If the data is nil/empty, it basically means there is an error somehow, in that case, the code should reflect which error it is, and data should be used to show more detailed message.

Copy link
Member Author

@lowzj lowzj Jun 11, 2020

Choose a reason for hiding this comment

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

ok, maybe we should provide a method to send header only.

}

// HandleErrorResponse handles err from server side and constructs response
// for client side.
func HandleErrorResponse(w http.ResponseWriter, err error) {
switch e := err.(type) {
case *errortypes.HTTPError:
_ = EncodeResponse(w, e.Code, errResp(e.Code, e.Msg))
_ = SendResponse(w, e.Code, errResp(e.Code, e.Msg))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is odd, The error should not be ignored silently, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@lowzj lowzj force-pushed the refactor-api-register branch 3 times, most recently from b823c27 to 63832e8 Compare June 11, 2020 08:03
}

// EncodeResponse encodes response in json.
// SendResponse encodes response in json.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ SendResponse/ EncodeResponse

@@ -47,6 +47,9 @@ func newCategory(name, prefix string) *category {
apiCategories[name] = &category{
name: name,
prefix: prefix,
handlerSpecs: []*HandlerSpec{
listHandler(name),
Copy link
Contributor

Choose a reason for hiding this comment

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

why should we add a default list handler here?

Copy link
Member Author

@lowzj lowzj Jun 30, 2020

Choose a reason for hiding this comment

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

It can help us to get all the available APIs provided by supernode.

Unverified

This user has not yet uploaded their public signing key.
Signed-off-by: lowzj <zj3142063@gmail.com>
@lowzj lowzj force-pushed the refactor-api-register branch from 63832e8 to cd9fa36 Compare June 30, 2020 18:01
@starnop
Copy link
Contributor

starnop commented Jul 2, 2020

LGTM.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants