config, api: Add Service Config #4869

CabinfeverB · 2022-04-28T06:14:55Z

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

What problem does this PR solve?

Issue Number: Ref #4480

What is changed and how does it work?

Add an independent config for audit and rate limit in the future

add service config

Check List

Tests

Unit test
Integration test

Code changes

Has configuration change
Has HTTP API interfaces changed (Don't forget to add the declarative for the new API)
Has persistent data change

Release note

None.

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

ti-chi-bot · 2022-04-28T06:14:56Z

[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-28T06:37:19Z

Codecov Report

Merging #4869 (7eadaf8) into master (3e95051) will increase coverage by 0.02%.
The diff coverage is 81.57%.

@@            Coverage Diff             @@
##           master    #4869      +/-   ##
==========================================
+ Coverage   75.37%   75.39%   +0.02%     
==========================================
  Files         298      302       +4     
  Lines       29554    29657     +103     
==========================================
+ Hits        22275    22360      +85     
- Misses       5334     5353      +19     
+ Partials     1945     1944       -1

Flag	Coverage Δ
unittests	`75.39% <81.57%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
server/api/min_resolved_ts.go	`100.00% <ø> (ø)`
server/api/service_gc_safepoint.go	`60.86% <ø> (ø)`
server/config/config.go	`78.08% <ø> (-0.10%)`	⬇️
server/config/persist_options.go	`93.72% <ø> (+0.71%)`	⬆️
server/storage/endpoint/key_path.go	`100.00% <ø> (ø)`
server/storage/storage.go	`70.27% <ø> (ø)`
server/storage/endpoint/service_middleware.go	`63.63% <63.63%> (ø)`
server/api/service_middleware.go	`68.18% <68.18%> (ø)`
server/config/service_middleware_config.go	`75.00% <75.00%> (ø)`
server/server.go	`72.81% <96.42%> (-0.01%)`	⬇️
... and 23 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 3e95051...7eadaf8. Read the comment docs.

rleungx · 2022-04-28T09:00:07Z

server/api/config.go

@@ -151,6 +151,8 @@ func (h *confHandler) updateConfig(cfg *config.Config, key string, value interfa
 			return errors.Errorf("cannot update config prefix %s", kp[0])
 		}
 		return h.updateReplicationModeConfig(cfg, kp[1:], value)
+	case "service":


Do we have a better naming?

Do we have a better naming?

how about "self-protection"

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

AndreMouche

Could you please use hyphens (-) to improve the readability of URIs ? ref https://restfulapi.net/resource-naming/

AndreMouche · 2022-05-10T03:33:27Z

server/api/router.go

@@ -282,6 +282,10 @@ func createRouter(prefix string, svr *server.Server) *mux.Router {
 	registerFunc(apiRouter, "/admin/persist-file/{file_name}", adminHandler.SavePersistFile, setMethods("POST"), setAuditBackend(localLog))
 	registerFunc(clusterRouter, "/admin/replication_mode/wait-async", adminHandler.UpdateWaitAsyncTime, setMethods("POST"), setAuditBackend(localLog))

+	selfProtectionHandler := newSelfProtectionHandler(svr, rd)
+	registerFunc(apiRouter, "/self_protection/config", selfProtectionHandler.GetSelfProtectionConfig, setMethods("GET"))


Suggested change

registerFunc(apiRouter, "/self_protection/config", selfProtectionHandler.GetSelfProtectionConfig, setMethods("GET"))

registerFunc(apiRouter, "/self-protection/config", selfProtectionHandler.GetSelfProtectionConfig, setMethods("GET"))

AndreMouche · 2022-05-10T03:35:26Z

server/api/router.go

@@ -282,6 +282,10 @@ func createRouter(prefix string, svr *server.Server) *mux.Router {
 	registerFunc(apiRouter, "/admin/persist-file/{file_name}", adminHandler.SavePersistFile, setMethods("POST"), setAuditBackend(localLog))
 	registerFunc(clusterRouter, "/admin/replication_mode/wait-async", adminHandler.UpdateWaitAsyncTime, setMethods("POST"), setAuditBackend(localLog))

+	selfProtectionHandler := newSelfProtectionHandler(svr, rd)
+	registerFunc(apiRouter, "/self_protection/config", selfProtectionHandler.GetSelfProtectionConfig, setMethods("GET"))
+	registerFunc(apiRouter, "/self_protection/config", selfProtectionHandler.SetSelfProtectionConfig, setMethods("POST"), setAuditBackend(localLog))


Suggested change

registerFunc(apiRouter, "/self_protection/config", selfProtectionHandler.SetSelfProtectionConfig, setMethods("POST"), setAuditBackend(localLog))

registerFunc(apiRouter, "/self-protection/config", selfProtectionHandler.SetSelfProtectionConfig, setMethods("POST"), setAuditBackend(localLog))

nolouch · 2022-05-10T03:41:45Z

server/api/router.go

@@ -282,6 +282,10 @@ func createRouter(prefix string, svr *server.Server) *mux.Router {
 	registerFunc(apiRouter, "/admin/persist-file/{file_name}", adminHandler.SavePersistFile, setMethods("POST"), setAuditBackend(localLog))
 	registerFunc(clusterRouter, "/admin/replication_mode/wait-async", adminHandler.UpdateWaitAsyncTime, setMethods("POST"), setAuditBackend(localLog))

+	selfProtectionHandler := newSelfProtectionHandler(svr, rd)
+	registerFunc(apiRouter, "/self_protection/config", selfProtectionHandler.GetSelfProtectionConfig, setMethods("GET"))


Can we change self_protection to service-middlewares or middlewares? IMO, self-protection is better to be the project name, not in the code.

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

nolouch

Overall LGTM. cc @rleungx

rleungx

The rest LGTM

rleungx · 2022-05-12T02:57:25Z

server/api/service_middleware.go

+	}
+}
+
+// @Tags service_middleware


better to keep the style with other interfaces

better to keep the style with other interfaces

How about changing all interfaces to Snake case？ I have changed service_gc_safepoint and min_resolved_ts

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

AndreMouche

Rest LGTM

AndreMouche · 2022-05-12T07:21:11Z

server/api/service_middleware.go

+		return
+	}
+
+	for k, v := range conf {


what if the conf is empty?

add a check

AndreMouche · 2022-05-12T07:22:04Z

server/api/service_middleware_test.go

+	addr := fmt.Sprintf("%s/service-middleware/config", s.urlPrefix)
+	ms := map[string]interface{}{
+		"enable-audit": "true",
+	}


How about add a case with an empty body or ms := map[string]interface{}{}

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

AndreMouche

LGTM

rleungx · 2022-05-17T10:02:57Z

/merge

ti-chi-bot · 2022-05-17T10:02:57Z

@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-05-17T10:02:59Z

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

Commit hash: 083b857

ti-chi-bot · 2022-05-17T10:03:12Z

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

ref #4666, ref #4839, ref #4869 update limiter config when reload presist config 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, ref tikv#4869 update limiter config when reload presist config 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>

add service config

900df00

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 Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 28, 2022

ti-chi-bot requested review from disksing and rleungx April 28, 2022 06:15

CabinfeverB changed the title ~~add service config~~ config, api: Add Service Config Apr 28, 2022

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Apr 28, 2022

add service config

133ab15

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

CabinfeverB mentioned this pull request Apr 28, 2022

Tracking issue for PD own service self-protection #4480

Closed

18 tasks

CabinfeverB marked this pull request as ready for review April 28, 2022 06:32

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 28, 2022

Merge branch 'master' into service_config

71bbf8c

CabinfeverB mentioned this pull request Apr 28, 2022

*: Add Limiter Config #4839

Merged

rleungx reviewed Apr 28, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 29, 2022

CabinfeverB added 4 commits May 5, 2022 14:44

merge master

7829f25

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

Merge branch 'master' into service_config

ba54cf8

add self protection config

720b051

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

add self protection config

6baf748

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

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2022

CabinfeverB added 3 commits May 9, 2022 18:52

add test

4565f91

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

add test

bfdd09b

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

add test

6c69db5

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

AndreMouche reviewed May 10, 2022

View reviewed changes

nolouch reviewed May 10, 2022

View reviewed changes

rename

1eded0e

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

CabinfeverB requested review from rleungx and nolouch May 11, 2022 05:28

nolouch reviewed May 11, 2022

View reviewed changes

rleungx reviewed May 12, 2022

View reviewed changes

change API tags to underline

aecd4d0

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

rleungx approved these changes May 12, 2022

View reviewed changes

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

AndreMouche reviewed May 12, 2022

View reviewed changes

add empty test

083b857

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

AndreMouche approved these changes May 13, 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 May 13, 2022

CabinfeverB requested a review from nolouch May 17, 2022 08:41

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

Merge branch 'master' into service_config

7eadaf8

ti-chi-bot merged commit dac3836 into tikv:master May 17, 2022

CabinfeverB mentioned this pull request May 18, 2022

server: add limiter config and reload mechanism #4842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config, api: Add Service Config #4869

config, api: Add Service Config #4869

CabinfeverB commented Apr 28, 2022 •

edited

Loading

ti-chi-bot commented Apr 28, 2022 •

edited

Loading

codecov bot commented Apr 28, 2022 •

edited

Loading

rleungx Apr 28, 2022

CabinfeverB Apr 29, 2022

AndreMouche left a comment

AndreMouche May 10, 2022

AndreMouche May 10, 2022

nolouch May 10, 2022

CabinfeverB May 10, 2022

nolouch left a comment

rleungx left a comment

rleungx May 12, 2022

CabinfeverB May 12, 2022

AndreMouche left a comment

AndreMouche May 12, 2022

CabinfeverB May 13, 2022

AndreMouche May 12, 2022

CabinfeverB May 13, 2022

AndreMouche left a comment

rleungx commented May 17, 2022

ti-chi-bot commented May 17, 2022

ti-chi-bot commented May 17, 2022

ti-chi-bot commented May 17, 2022

	registerFunc(apiRouter, "/self_protection/config", selfProtectionHandler.GetSelfProtectionConfig, setMethods("GET"))
	registerFunc(apiRouter, "/self-protection/config", selfProtectionHandler.GetSelfProtectionConfig, setMethods("GET"))

config, api: Add Service Config #4869

config, api: Add Service Config #4869

Conversation

CabinfeverB commented Apr 28, 2022 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Apr 28, 2022 • edited Loading

codecov bot commented Apr 28, 2022 • edited Loading

Codecov Report

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

nolouch left a comment

Choose a reason for hiding this comment

rleungx 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

AndreMouche left a comment

Choose a reason for hiding this comment

rleungx commented May 17, 2022

ti-chi-bot commented May 17, 2022

ti-chi-bot commented May 17, 2022

ti-chi-bot commented May 17, 2022

CabinfeverB commented Apr 28, 2022 •

edited

Loading

ti-chi-bot commented Apr 28, 2022 •

edited

Loading

codecov bot commented Apr 28, 2022 •

edited

Loading