Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: use middleware to get raftCluster #1908

Merged
merged 4 commits into from
Nov 7, 2019
Merged

Conversation

disksing
Copy link
Contributor

@disksing disksing commented Nov 7, 2019

What problem does this PR solve?

Many handlers need to get RaftCluster and check if the cluster is bootstrapped.
Fix #1905

What is changed and how it works?

Use middleware to eliminate duplicated code.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
manual test detail
  • run pd
  • curl /stores /store/1, got not bootstrapped error
  • run tikv
  • curl /stores /store/1, no error

@disksing disksing added the component/api HTTP API. label Nov 7, 2019
@codecov-io
Copy link

codecov-io commented Nov 7, 2019

Codecov Report

Merging #1908 into master will increase coverage by 0.53%.
The diff coverage is 91.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1908      +/-   ##
==========================================
+ Coverage   77.33%   77.87%   +0.53%     
==========================================
  Files         165      167       +2     
  Lines       16590    16530      -60     
==========================================
+ Hits        12830    12872      +42     
+ Misses       2750     2665      -85     
+ Partials     1010      993      -17
Impacted Files Coverage Δ
server/api/rule.go 3.96% <0%> (+0.68%) ⬆️
server/api/stats.go 100% <100%> (+25%) ⬆️
server/api/context.go 100% <100%> (ø)
server/api/label.go 86.66% <100%> (+7.87%) ⬆️
server/api/admin.go 75.75% <100%> (+6.31%) ⬆️
server/api/router.go 100% <100%> (ø) ⬆️
server/api/store.go 66.39% <100%> (+5.2%) ⬆️
server/api/middleware.go 75% <75%> (ø)
server/api/region.go 76.69% <87.5%> (+8%) ⬆️
server/grpc_service.go 57.32% <0%> (-1.3%) ⬇️
... and 11 more

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 926b46e...892ccb6. Read the comment docs.

@@ -28,127 +28,130 @@ func createRouter(prefix string, svr *server.Server) *mux.Router {
IndentJSON: true,
})

router := mux.NewRouter().PathPrefix(prefix).Subrouter()
root := mux.NewRouter().PathPrefix(prefix).Subrouter()
Copy link
Contributor

Choose a reason for hiding this comment

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

why rename it and what difference between root and clusterRouter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

clusterRouter is a sub router of root which check the cluster is bootstrapped for all registered handlers.
Rename router to root is because there are 2 routers in the function so I think it's better to give another name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe rename to rootRouter?

return context.WithValue(ctx, clusterKey, cluster)
}

func getCtxCluster(ctx context.Context) *server.RaftCluster {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about getCluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good

server/api/middleware.go Show resolved Hide resolved
Signed-off-by: disksing <[email protected]>
Copy link
Contributor

@shafreeck shafreeck left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@rleungx rleungx left a comment

Choose a reason for hiding this comment

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

LGTM.

@disksing
Copy link
Contributor Author

disksing commented Nov 7, 2019

/merge

@sre-bot sre-bot added the status/can-merge Indicates a PR has been approved by a committer. label Nov 7, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 7, 2019

/run-all-tests

@sre-bot sre-bot merged commit 5cfd3b1 into tikv:master Nov 7, 2019
@disksing disksing deleted the middleware branch November 7, 2019 08:16
@HuSharp HuSharp mentioned this pull request Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/api HTTP API. status/can-merge Indicates a PR has been approved by a committer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

api: use mux.Middleware to eliminate duplicate checks
6 participants