api: add Rate-limit config update API #4843

CabinfeverB · 2022-04-25T05:08:19Z

Signed-off-by: Cabinfever_B cabinfeveroier@gmail.com

What problem does this PR solve?

Issue Number: Ref #4666
This PR should be merged after #4839

What is changed and how it works?

add Rate-limit config update API

add Rate-limit config update API

Check List

Tests

Unit test

Code changes

Has HTTP API interfaces change (Don't forget to add the declarative for API)

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2022-04-25T05:08:20Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

AndreMouche
rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

codecov · 2022-04-25T05:45:46Z

Codecov Report

Merging #4843 (3774d3b) into master (c628ff9) will increase coverage by 0.14%.
The diff coverage is 89.62%.

@@            Coverage Diff             @@
##           master    #4843      +/-   ##
==========================================
+ Coverage   75.64%   75.78%   +0.14%     
==========================================
  Files         309      310       +1     
  Lines       30598    30664      +66     
==========================================
+ Hits        23145    23238      +93     
+ Misses       5453     5428      -25     
+ Partials     2000     1998       -2

Flag	Coverage Δ
unittests	`75.78% <89.62%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/jsonutil/jsonutil.go	`73.33% <73.33%> (ø)`
server/server.go	`73.60% <87.50%> (-0.38%)`	⬇️
server/api/service_middleware.go	`84.82% <92.85%> (+15.85%)`	⬆️
server/api/config.go	`61.50% <100.00%> (+0.13%)`	⬆️
server/api/router.go	`97.83% <100.00%> (+<0.01%)`	⬆️
server/tso/tso.go	`67.79% <0.00%> (-6.78%)`	⬇️
pkg/dashboard/adapter/manager.go	`75.86% <0.00%> (-5.75%)`	⬇️
server/schedule/labeler/rules.go	`87.50% <0.00%> (-2.28%)`	⬇️
pkg/etcdutil/etcdutil.go	`84.88% <0.00%> (-1.17%)`	⬇️
server/config/persist_options.go	`93.37% <0.00%> (-0.70%)`	⬇️
... and 19 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 c628ff9...3774d3b. Read the comment docs.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

AndreMouche

Rest LGTM

AndreMouche · 2022-06-14T07:12:34Z

server/api/service_middleware.go

+		if err != nil {
+			h.rd.JSON(w, http.StatusInternalServerError, err.Error())
+		} else {
+			h.rd.JSON(w, http.StatusOK, fmt.Sprintf("%s %s", concurrencyUpdatedFlag, qpsRateUpdatedFlag))


Suggested change

h.rd.JSON(w, http.StatusOK, fmt.Sprintf("%s %s", concurrencyUpdatedFlag, qpsRateUpdatedFlag))

h.rd.JSON(w, http.StatusOK, fmt.Sprintf("%s\n%s", concurrencyUpdatedFlag, qpsRateUpdatedFlag))

How about returns the new ratelimit config once set success?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

AndreMouche

LGTM

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-06-14T09:03:58Z

server/api/router.go

@@ -285,6 +285,7 @@ func createRouter(prefix string, svr *server.Server) *mux.Router {
 	serviceMiddlewareHandler := newServiceMiddlewareHandler(svr, rd)
 	registerFunc(apiRouter, "/service-middleware/config", serviceMiddlewareHandler.GetServiceMiddlewareConfig, setMethods("GET"))
 	registerFunc(apiRouter, "/service-middleware/config", serviceMiddlewareHandler.SetServiceMiddlewareConfig, setMethods("POST"), setAuditBackend(localLog))
+	registerFunc(apiRouter, "/service-middleware/rate-limit/config", serviceMiddlewareHandler.SetRatelimitConfig, setMethods("POST"), setAuditBackend(localLog))


Do we need it?

/service-middleware/rate-limit/config is used to update Incrementally. If users want to overwrite config, we can use /service-middleware/config

rleungx · 2022-06-14T09:05:16Z

server/api/service_middleware.go

+	}
+	// update qps rate limiter
+	qpsRateUpdatedFlag := "QPS rate limiter is not changed."
+	qps, okq := input["qps"].(float64)


if QPS is less than 0, does it belong to an error?

It means delete QPS limit

server/api/service_middleware.go

rleungx · 2022-06-14T09:06:37Z

server/api/service_middleware.go

+}
+
+type rateLimitResult struct {
+	ConcurrencyUpdatedFlag string                               `json:"Concurrency"`


use small letter?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-06-15T03:45:58Z

server/api/service_middleware.go

-	ConcurrencyUpdatedFlag string                               `json:"Concurrency"`
-	QPSRateUpdatedFlag     string                               `json:"QPS"`
+	ConcurrencyUpdatedFlag string                               `json:"concurrency"`
+	QPSRateUpdatedFlag     string                               `json:"qps"`
 	LimiterConfig          map[string]ratelimit.DimensionConfig `json:"new-config"`


Suggested change

LimiterConfig map[string]ratelimit.DimensionConfig `json:"new-config"`

LimiterConfig map[string]ratelimit.DimensionConfig `json:"limiter-config"`

rleungx · 2022-06-15T03:47:11Z

server/api/service_middleware.go

+// @Param body body object string "json params"
+// @Produce json
+// @Success 200 {string} string ""
+// @Failure 400 {string} string ""


Suggested change

// @Failure 400 {string} string ""

// @Failure 400 {string} string "The input is invalid."

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-06-15T04:07:56Z

server/api/service_middleware.go

+		concurrency := uint64(concurrencyFloat)
+		cfg.ConcurrencyLimit = concurrency
+	}


Suggested change

concurrency := uint64(concurrencyFloat)

cfg.ConcurrencyLimit = concurrency

}

cfg.ConcurrencyLimit = uint64(concurrencyFloat)

}

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx · 2022-06-15T09:09:08Z

/merge

ti-chi-bot · 2022-06-15T09:09:09Z

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-06-15T09:09:11Z

This pull request has been accepted and is ready to merge.

Commit hash: d558575

ti-chi-bot · 2022-06-15T09:09:23Z

@CabinfeverB: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

close #4666, ref #4839, ref #4840, ref #4842, ref #4843 Add rate limit middleware for HTTP API. Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

ref tikv#4666, ref tikv#4839 add Rate-limit config update API Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>

close tikv#4666, ref tikv#4839, ref tikv#4840, ref tikv#4842, ref tikv#4843 Add rate limit middleware for HTTP API. Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> Co-authored-by: Ryan Leung <rleungx@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> --wip-- [skip ci]

CabinfeverB added 6 commits April 24, 2022 16:45

add limit config

e8102b4

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add rate-limit config

b9bcfbf

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix test

1a93604

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

remove future code

cefebc9

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add rate-limit config presist reload

826ef7f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

add rate-limit config api

8f24203

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot added do-not-merge/needs-linked-issue do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note The PR should write the release note. labels Apr 25, 2022

ti-chi-bot requested review from nolouch and rleungx April 25, 2022 05:08

CabinfeverB added 3 commits April 25, 2022 13:18

fix test

176d73b

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'master' into rata_limit_config_api

fdbf344

fix comment

0d73c53

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB added 7 commits April 25, 2022 15:18

add test

3318100

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix comment

4e2a840

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'limiter_config' into server_rate_limit

6d1c289

Merge branch 'limiter_config' into rata_limit_config_api

001cb93

Merge branch 'master' into limiter_config

713c8a8

Merge branch 'limiter_config' into rata_limit_config_api

1744bd6

add test

456d504

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB changed the title ~~Rata limit config api~~ api: add Rate-limit config update API Apr 25, 2022

CabinfeverB marked this pull request as ready for review April 25, 2022 08:28

ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 25, 2022

CabinfeverB mentioned this pull request Apr 25, 2022

server, api: add rate-limit middleware #4844

Merged

CabinfeverB added 2 commits April 26, 2022 01:28

fix typo

049c4d6

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix typo

1b183a0

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot removed the do-not-merge/needs-linked-issue label Apr 26, 2022

fix

1fedfbc

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

AndreMouche reviewed Jun 14, 2022

View reviewed changes

CabinfeverB added 2 commits June 14, 2022 16:11

fix

e9ee0e2

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix

dd70f46

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

AndreMouche approved these changes Jun 14, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jun 14, 2022

CabinfeverB added 2 commits June 14, 2022 16:48

fix type

b15ac5a

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix typo

2d9f124

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Jun 14, 2022

View reviewed changes

CabinfeverB added 2 commits June 14, 2022 17:54

change url path

97d1b9e

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

fix

fa28dd4

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Jun 15, 2022

View reviewed changes

fix

aca84d7

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx reviewed Jun 15, 2022

View reviewed changes

fix

d558575

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

rleungx approved these changes Jun 15, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jun 15, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jun 15, 2022

Merge branch 'master' into rata_limit_config_api

3774d3b

ti-chi-bot merged commit 0da658c into tikv:master Jun 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: add Rate-limit config update API #4843

api: add Rate-limit config update API #4843

CabinfeverB commented Apr 25, 2022 •

edited

ti-chi-bot commented Apr 25, 2022 •

edited

codecov bot commented Apr 25, 2022 •

edited

AndreMouche left a comment

AndreMouche Jun 14, 2022

AndreMouche Jun 14, 2022

AndreMouche left a comment

rleungx Jun 14, 2022

CabinfeverB Jun 14, 2022

rleungx Jun 14, 2022

CabinfeverB Jun 14, 2022

rleungx Jun 14, 2022

rleungx Jun 15, 2022

rleungx Jun 15, 2022

rleungx Jun 15, 2022

rleungx commented Jun 15, 2022

ti-chi-bot commented Jun 15, 2022

ti-chi-bot commented Jun 15, 2022

ti-chi-bot commented Jun 15, 2022

	h.rd.JSON(w, http.StatusOK, fmt.Sprintf("%s %s", concurrencyUpdatedFlag, qpsRateUpdatedFlag))
	h.rd.JSON(w, http.StatusOK, fmt.Sprintf("%s\n%s", concurrencyUpdatedFlag, qpsRateUpdatedFlag))

	LimiterConfig map[string]ratelimit.DimensionConfig `json:"new-config"`
	LimiterConfig map[string]ratelimit.DimensionConfig `json:"limiter-config"`

	// @Failure 400 {string} string ""
	// @Failure 400 {string} string "The input is invalid."

api: add Rate-limit config update API #4843

api: add Rate-limit config update API #4843

Conversation

CabinfeverB commented Apr 25, 2022 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Apr 25, 2022 • edited

codecov bot commented Apr 25, 2022 • edited

Codecov Report

AndreMouche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreMouche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rleungx commented Jun 15, 2022

ti-chi-bot commented Jun 15, 2022

ti-chi-bot commented Jun 15, 2022

ti-chi-bot commented Jun 15, 2022

CabinfeverB commented Apr 25, 2022 •

edited

ti-chi-bot commented Apr 25, 2022 •

edited

codecov bot commented Apr 25, 2022 •

edited