From 1a1e0b79e96422da03e2e69f2b132e87aac4712d Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Tue, 14 Nov 2023 17:37:20 +0800 Subject: [PATCH 1/5] tests: make TestResetTS stable Signed-off-by: lhy1024 --- server/api/admin_test.go | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/server/api/admin_test.go b/server/api/admin_test.go index 09130fd8385..f9b866af952 100644 --- a/server/api/admin_test.go +++ b/server/api/admin_test.go @@ -18,6 +18,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "testing" "time" @@ -27,6 +28,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/tikv/pd/pkg/core" "github.com/tikv/pd/pkg/replication" + "github.com/tikv/pd/pkg/utils/apiutil" tu "github.com/tikv/pd/pkg/utils/testutil" "github.com/tikv/pd/server" ) @@ -188,9 +190,24 @@ func (suite *adminTestSuite) TestResetTS() { values, err := json.Marshal(args) suite.NoError(err) re := suite.Require() - err = tu.CheckPostJSON(testDialClient, url, values, - tu.StatusOK(re), - tu.StringEqual(re, "\"Reset ts successfully.\"\n")) + tu.Eventually(re, func() bool { + resp, err := apiutil.PostJSON(testDialClient, url, values) + re.NoError(err) + defer resp.Body.Close() + b, err := io.ReadAll(resp.Body) + re.NoError(err) + switch resp.StatusCode { + case http.StatusOK: + re.Contains(string(b), "Reset ts successfully.") + return true + case http.StatusForbidden: + re.Contains(string(b), "[PD:etcd:ErrEtcdTxnConflict]etcd transaction failed, conflicted and rolled back") + return false + default: + re.FailNow("unexpected status code %d", resp.StatusCode) + return false + } + }) suite.NoError(err) t2 := makeTS(32 * time.Hour) args["tso"] = fmt.Sprintf("%d", t2) From 7a56c165e872d3d5fb665d91430cf02fc99daf1d Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Thu, 16 Nov 2023 14:13:12 +0800 Subject: [PATCH 2/5] add status code Signed-off-by: lhy1024 --- server/api/admin_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/server/api/admin_test.go b/server/api/admin_test.go index f9b866af952..0418f0849af 100644 --- a/server/api/admin_test.go +++ b/server/api/admin_test.go @@ -203,6 +203,9 @@ func (suite *adminTestSuite) TestResetTS() { case http.StatusForbidden: re.Contains(string(b), "[PD:etcd:ErrEtcdTxnConflict]etcd transaction failed, conflicted and rolled back") return false + case http.StatusInternalServerError: + re.Contains(string(b), "redirect to not leader") + return false default: re.FailNow("unexpected status code %d", resp.StatusCode) return false From 871a8d96da778adbebcb6f85149eb4501b8e259e Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Thu, 16 Nov 2023 14:20:02 +0800 Subject: [PATCH 3/5] fix status code Signed-off-by: lhy1024 --- pkg/mcs/tso/server/apis/v1/api.go | 2 ++ pkg/tso/admin.go | 2 ++ server/api/admin_test.go | 5 +---- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/mcs/tso/server/apis/v1/api.go b/pkg/mcs/tso/server/apis/v1/api.go index 1b8f68778af..9fb56099617 100644 --- a/pkg/mcs/tso/server/apis/v1/api.go +++ b/pkg/mcs/tso/server/apis/v1/api.go @@ -185,6 +185,8 @@ func ResetTS(c *gin.Context) { if err = svr.ResetTS(ts, ignoreSmaller, skipUpperBoundCheck, 0); err != nil { if err == errs.ErrServerNotStarted { c.String(http.StatusInternalServerError, err.Error()) + } else if err == errs.ErrEtcdTxnConflict { + c.String(http.StatusServiceUnavailable, err.Error()) } else { c.String(http.StatusForbidden, err.Error()) } diff --git a/pkg/tso/admin.go b/pkg/tso/admin.go index 7d510cdef65..3ec2132a3f6 100644 --- a/pkg/tso/admin.go +++ b/pkg/tso/admin.go @@ -96,6 +96,8 @@ func (h *AdminHandler) ResetTS(w http.ResponseWriter, r *http.Request) { if err = handler.ResetTS(ts, ignoreSmaller, skipUpperBoundCheck, 0); err != nil { if err == errs.ErrServerNotStarted { h.rd.JSON(w, http.StatusInternalServerError, err.Error()) + } else if err == errs.ErrEtcdTxnConflict { + h.rd.JSON(w, http.StatusServiceUnavailable, err.Error()) } else { h.rd.JSON(w, http.StatusForbidden, err.Error()) } diff --git a/server/api/admin_test.go b/server/api/admin_test.go index 0418f0849af..ad761d532ea 100644 --- a/server/api/admin_test.go +++ b/server/api/admin_test.go @@ -200,12 +200,9 @@ func (suite *adminTestSuite) TestResetTS() { case http.StatusOK: re.Contains(string(b), "Reset ts successfully.") return true - case http.StatusForbidden: + case http.StatusServiceUnavailable: re.Contains(string(b), "[PD:etcd:ErrEtcdTxnConflict]etcd transaction failed, conflicted and rolled back") return false - case http.StatusInternalServerError: - re.Contains(string(b), "redirect to not leader") - return false default: re.FailNow("unexpected status code %d", resp.StatusCode) return false From 50e03654f7a3322ef093e1f40b5ccdf6e38ec296 Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Thu, 16 Nov 2023 14:29:06 +0800 Subject: [PATCH 4/5] add comments Signed-off-by: lhy1024 --- pkg/mcs/tso/server/apis/v1/api.go | 3 +++ pkg/tso/admin.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/pkg/mcs/tso/server/apis/v1/api.go b/pkg/mcs/tso/server/apis/v1/api.go index 9fb56099617..817fd04f1f7 100644 --- a/pkg/mcs/tso/server/apis/v1/api.go +++ b/pkg/mcs/tso/server/apis/v1/api.go @@ -186,6 +186,9 @@ func ResetTS(c *gin.Context) { if err == errs.ErrServerNotStarted { c.String(http.StatusInternalServerError, err.Error()) } else if err == errs.ErrEtcdTxnConflict { + // If the error is ErrEtcdTxnConflict, it means there is a temporary failure. + // Return 503 to let the client retry. + // Ref: https://datatracker.ietf.org/doc/html/rfc7231#section-6.6.4 c.String(http.StatusServiceUnavailable, err.Error()) } else { c.String(http.StatusForbidden, err.Error()) diff --git a/pkg/tso/admin.go b/pkg/tso/admin.go index 3ec2132a3f6..15b0415a422 100644 --- a/pkg/tso/admin.go +++ b/pkg/tso/admin.go @@ -97,6 +97,9 @@ func (h *AdminHandler) ResetTS(w http.ResponseWriter, r *http.Request) { if err == errs.ErrServerNotStarted { h.rd.JSON(w, http.StatusInternalServerError, err.Error()) } else if err == errs.ErrEtcdTxnConflict { + // If the error is ErrEtcdTxnConflict, it means there is a temporary failure. + // Return 503 to let the client retry. + // Ref: https://datatracker.ietf.org/doc/html/rfc7231#section-6.6.4 h.rd.JSON(w, http.StatusServiceUnavailable, err.Error()) } else { h.rd.JSON(w, http.StatusForbidden, err.Error()) From 23fa5f0b01e52bf4b7cc1707ea8f7d53db4b2f38 Mon Sep 17 00:00:00 2001 From: lhy1024 Date: Thu, 16 Nov 2023 18:59:29 +0800 Subject: [PATCH 5/5] add comments Signed-off-by: lhy1024 --- pkg/mcs/tso/server/apis/v1/api.go | 5 ++++- pkg/tso/admin.go | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/mcs/tso/server/apis/v1/api.go b/pkg/mcs/tso/server/apis/v1/api.go index 817fd04f1f7..33e1e0801aa 100644 --- a/pkg/mcs/tso/server/apis/v1/api.go +++ b/pkg/mcs/tso/server/apis/v1/api.go @@ -15,6 +15,7 @@ package apis import ( + "fmt" "net/http" "strconv" "sync" @@ -150,6 +151,7 @@ type ResetTSParams struct { // @Failure 400 {string} string "The input is invalid." // @Failure 403 {string} string "Reset ts is forbidden." // @Failure 500 {string} string "TSO server failed to proceed the request." +// @Failure 503 {string} string "It's a temporary failure, please retry." // @Router /admin/reset-ts [post] // if force-use-larger=true: // @@ -189,7 +191,8 @@ func ResetTS(c *gin.Context) { // If the error is ErrEtcdTxnConflict, it means there is a temporary failure. // Return 503 to let the client retry. // Ref: https://datatracker.ietf.org/doc/html/rfc7231#section-6.6.4 - c.String(http.StatusServiceUnavailable, err.Error()) + c.String(http.StatusServiceUnavailable, + fmt.Sprintf("It's a temporary failure with error %s, please retry.", err.Error())) } else { c.String(http.StatusForbidden, err.Error()) } diff --git a/pkg/tso/admin.go b/pkg/tso/admin.go index 15b0415a422..f19d8e71d05 100644 --- a/pkg/tso/admin.go +++ b/pkg/tso/admin.go @@ -15,6 +15,7 @@ package tso import ( + "fmt" "net/http" "strconv" @@ -53,6 +54,7 @@ func NewAdminHandler(handler Handler, rd *render.Render) *AdminHandler { // @Failure 400 {string} string "The input is invalid." // @Failure 403 {string} string "Reset ts is forbidden." // @Failure 500 {string} string "TSO server failed to proceed the request." +// @Failure 503 {string} string "It's a temporary failure, please retry." // @Router /admin/reset-ts [post] // if force-use-larger=true: // @@ -100,7 +102,8 @@ func (h *AdminHandler) ResetTS(w http.ResponseWriter, r *http.Request) { // If the error is ErrEtcdTxnConflict, it means there is a temporary failure. // Return 503 to let the client retry. // Ref: https://datatracker.ietf.org/doc/html/rfc7231#section-6.6.4 - h.rd.JSON(w, http.StatusServiceUnavailable, err.Error()) + h.rd.JSON(w, http.StatusServiceUnavailable, + fmt.Sprintf("It's a temporary failure with error %s, please retry.", err.Error())) } else { h.rd.JSON(w, http.StatusForbidden, err.Error()) }