From f91acdb99009e3b7367235d31cf1fdcd9ef98416 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Wed, 27 Dec 2023 17:25:18 +0800 Subject: [PATCH 1/4] fix panic when region doesn't have a leader Signed-off-by: Ryan Leung --- server/api/region.go | 47 +++++++++++++++++++++++++-- tests/pdctl/region/region_test.go | 53 +++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 2 deletions(-) diff --git a/server/api/region.go b/server/api/region.go index 4e6679a376d..b5e548b5224 100644 --- a/server/api/region.go +++ b/server/api/region.go @@ -257,7 +257,13 @@ func (h *regionHandler) GetRegionByID(w http.ResponseWriter, r *http.Request) { } regionInfo := rc.GetRegion(regionID) - h.rd.JSON(w, http.StatusOK, NewAPIRegionInfo(regionInfo)) + b, err := marshalRegionInfoJSON(r.Context(), regionInfo) + if err != nil { + h.rd.JSON(w, http.StatusInternalServerError, err.Error()) + return + } + + h.rd.Data(w, http.StatusOK, b) } // @Tags region @@ -276,7 +282,13 @@ func (h *regionHandler) GetRegion(w http.ResponseWriter, r *http.Request) { return } regionInfo := rc.GetRegionByKey([]byte(key)) - h.rd.JSON(w, http.StatusOK, NewAPIRegionInfo(regionInfo)) + b, err := marshalRegionInfoJSON(r.Context(), regionInfo) + if err != nil { + h.rd.JSON(w, http.StatusInternalServerError, err.Error()) + return + } + + h.rd.Data(w, http.StatusOK, b) } // @Tags region @@ -336,6 +348,37 @@ func newRegionsHandler(svr *server.Server, rd *render.Render) *regionsHandler { } } +// marshalRegionInfoJSON marshals region to bytes in `RegionInfo`'s JSON format. +// It is used to reduce the cost of JSON serialization. +func marshalRegionInfoJSON(ctx context.Context, r *core.RegionInfo) ([]byte, error) { + out := &jwriter.Writer{} + + region := &RegionInfo{} + select { + case <-ctx.Done(): + // Return early, avoid the unnecessary computation. + // See more details in https://github.com/tikv/pd/issues/6835 + return nil, ctx.Err() + default: + } + + InitRegion(r, region) + // EasyJSON will not check anonymous struct pointer field and will panic if the field is nil. + // So we need to set the field to default value explicitly when the anonymous struct pointer is nil. + region.Leader.setDefaultIfNil() + for i := range region.Peers { + region.Peers[i].setDefaultIfNil() + } + for i := range region.PendingPeers { + region.PendingPeers[i].setDefaultIfNil() + } + for i := range region.DownPeers { + region.DownPeers[i].setDefaultIfNil() + } + region.MarshalEasyJSON(out) + return out.Buffer.BuildBytes(), out.Error +} + // marshalRegionsInfoJSON marshals regions to bytes in `RegionsInfo`'s JSON format. // It is used to reduce the cost of JSON serialization. func marshalRegionsInfoJSON(ctx context.Context, regions []*core.RegionInfo) ([]byte, error) { diff --git a/tests/pdctl/region/region_test.go b/tests/pdctl/region/region_test.go index d56463d728d..764d13151fb 100644 --- a/tests/pdctl/region/region_test.go +++ b/tests/pdctl/region/region_test.go @@ -208,3 +208,56 @@ func TestRegion(t *testing.T) { {core.HexRegionKeyStr(r5.GetEndKey()), ""}, }, *rangeHoles) } + +func TestRegionNoLeader(t *testing.T) { + re := require.New(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cluster, err := tests.NewTestCluster(ctx, 1) + re.NoError(err) + err = cluster.RunInitialServers() + re.NoError(err) + cluster.WaitLeader() + url := cluster.GetConfig().GetClientURL() + stores := []*metapb.Store{ + { + Id: 1, + State: metapb.StoreState_Up, + LastHeartbeat: time.Now().UnixNano(), + }, + { + Id: 2, + State: metapb.StoreState_Up, + LastHeartbeat: time.Now().UnixNano(), + }, + { + Id: 3, + State: metapb.StoreState_Up, + LastHeartbeat: time.Now().UnixNano(), + }, + } + + leaderServer := cluster.GetLeaderServer() + re.NoError(leaderServer.BootstrapCluster()) + for i := 0; i < len(stores); i++ { + tests.MustPutStore(re, cluster, stores[i]) + } + + metaRegion := &metapb.Region{ + Id: 100, + StartKey: []byte(""), + EndKey: []byte(""), + Peers: []*metapb.Peer{ + {Id: 1, StoreId: 1}, + {Id: 5, StoreId: 2}, + {Id: 6, StoreId: 3}}, + RegionEpoch: &metapb.RegionEpoch{ConfVer: 1, Version: 1}, + } + r := core.NewRegionInfo(metaRegion, nil) + + cluster.GetLeaderServer().GetRaftCluster().GetBasicCluster().SetRegion(r) + + cmd := pdctlCmd.GetRootCmd() + _, err = pdctl.ExecuteCommand(cmd, "-u", url, "region", "100") + re.NoError(err) +} From 4ccb2e96d34a2c74a4840277d9ae55aa9bae8659 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Thu, 28 Dec 2023 14:04:53 +0800 Subject: [PATCH 2/4] address the comment Signed-off-by: Ryan Leung --- server/api/region.go | 47 ++++++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/server/api/region.go b/server/api/region.go index b5e548b5224..e81d4f0b1db 100644 --- a/server/api/region.go +++ b/server/api/region.go @@ -362,20 +362,7 @@ func marshalRegionInfoJSON(ctx context.Context, r *core.RegionInfo) ([]byte, err default: } - InitRegion(r, region) - // EasyJSON will not check anonymous struct pointer field and will panic if the field is nil. - // So we need to set the field to default value explicitly when the anonymous struct pointer is nil. - region.Leader.setDefaultIfNil() - for i := range region.Peers { - region.Peers[i].setDefaultIfNil() - } - for i := range region.PendingPeers { - region.PendingPeers[i].setDefaultIfNil() - } - for i := range region.DownPeers { - region.DownPeers[i].setDefaultIfNil() - } - region.MarshalEasyJSON(out) + covertAPIRegionInfo(r, region, out) return out.Buffer.BuildBytes(), out.Error } @@ -402,20 +389,7 @@ func marshalRegionsInfoJSON(ctx context.Context, regions []*core.RegionInfo) ([] if i > 0 { out.RawByte(',') } - InitRegion(r, region) - // EasyJSON will not check anonymous struct pointer field and will panic if the field is nil. - // So we need to set the field to default value explicitly when the anonymous struct pointer is nil. - region.Leader.setDefaultIfNil() - for i := range region.Peers { - region.Peers[i].setDefaultIfNil() - } - for i := range region.PendingPeers { - region.PendingPeers[i].setDefaultIfNil() - } - for i := range region.DownPeers { - region.DownPeers[i].setDefaultIfNil() - } - region.MarshalEasyJSON(out) + covertAPIRegionInfo(r, region, out) } out.RawByte(']') @@ -423,6 +397,23 @@ func marshalRegionsInfoJSON(ctx context.Context, regions []*core.RegionInfo) ([] return out.Buffer.BuildBytes(), out.Error } +func covertAPIRegionInfo(r *core.RegionInfo, region *RegionInfo, out *jwriter.Writer) { + InitRegion(r, region) + // EasyJSON will not check anonymous struct pointer field and will panic if the field is nil. + // So we need to set the field to default value explicitly when the anonymous struct pointer is nil. + region.Leader.setDefaultIfNil() + for i := range region.Peers { + region.Peers[i].setDefaultIfNil() + } + for i := range region.PendingPeers { + region.PendingPeers[i].setDefaultIfNil() + } + for i := range region.DownPeers { + region.DownPeers[i].setDefaultIfNil() + } + region.MarshalEasyJSON(out) +} + // @Tags region // @Summary List all regions in the cluster. // @Produce json From fab13816686bfe404feaf293c73c300297354615 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Tue, 2 Jan 2024 14:19:45 +0800 Subject: [PATCH 3/4] fix the conflicts Signed-off-by: Ryan Leung --- tests/pdctl/region/region_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/pdctl/region/region_test.go b/tests/pdctl/region/region_test.go index 764d13151fb..1a5b5006499 100644 --- a/tests/pdctl/region/region_test.go +++ b/tests/pdctl/region/region_test.go @@ -213,7 +213,7 @@ func TestRegionNoLeader(t *testing.T) { re := require.New(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - cluster, err := tests.NewTestCluster(ctx, 1) + cluster, err := pdTests.NewTestCluster(ctx, 1) re.NoError(err) err = cluster.RunInitialServers() re.NoError(err) @@ -240,7 +240,7 @@ func TestRegionNoLeader(t *testing.T) { leaderServer := cluster.GetLeaderServer() re.NoError(leaderServer.BootstrapCluster()) for i := 0; i < len(stores); i++ { - tests.MustPutStore(re, cluster, stores[i]) + pdTests.MustPutStore(re, cluster, stores[i]) } metaRegion := &metapb.Region{ @@ -257,7 +257,7 @@ func TestRegionNoLeader(t *testing.T) { cluster.GetLeaderServer().GetRaftCluster().GetBasicCluster().SetRegion(r) - cmd := pdctlCmd.GetRootCmd() - _, err = pdctl.ExecuteCommand(cmd, "-u", url, "region", "100") + cmd := ctl.GetRootCmd() + _, err = tests.ExecuteCommand(cmd, "-u", url, "region", "100") re.NoError(err) } From 6639f9db0859547f9b3e714192d687e17757fa5e Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Tue, 23 Jan 2024 15:57:04 +0800 Subject: [PATCH 4/4] resolve the conflicts Signed-off-by: Ryan Leung --- client/client.go | 2 +- server/api/rule_test.go | 8 ++++---- tests/pdctl/region/region_test.go | 12 ++++++------ 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/client/client.go b/client/client.go index daf3eac7968..f0d51ef2596 100644 --- a/client/client.go +++ b/client/client.go @@ -795,7 +795,7 @@ func (c *client) ScanRegions(ctx context.Context, key, endKey []byte, limit int) defer span.Finish() } start := time.Now() - defer cmdDurationScanRegions.Observe(time.Since(start).Seconds()) + defer func() { cmdDurationScanRegions.Observe(time.Since(start).Seconds()) }() var cancel context.CancelFunc scanCtx := ctx diff --git a/server/api/rule_test.go b/server/api/rule_test.go index d2000eb9562..8233e96f463 100644 --- a/server/api/rule_test.go +++ b/server/api/rule_test.go @@ -185,7 +185,7 @@ func (suite *ruleTestSuite) TestSet() { } func (suite *ruleTestSuite) TestGet() { - rule := placement.Rule{GroupID: "a", ID: "20", StartKeyHex: "1111", EndKeyHex: "3333", Role: "voter", Count: 1} + rule := &placement.Rule{GroupID: "a", ID: "20", StartKeyHex: "1111", EndKeyHex: "3333", Role: "voter", Count: 1} data, err := json.Marshal(rule) suite.NoError(err) re := suite.Require() @@ -194,7 +194,7 @@ func (suite *ruleTestSuite) TestGet() { testCases := []struct { name string - rule placement.Rule + rule *placement.Rule found bool code int }{ @@ -206,7 +206,7 @@ func (suite *ruleTestSuite) TestGet() { }, { name: "not found", - rule: placement.Rule{GroupID: "a", ID: "30", StartKeyHex: "1111", EndKeyHex: "3333", Role: "voter", Count: 1}, + rule: &placement.Rule{GroupID: "a", ID: "30", StartKeyHex: "1111", EndKeyHex: "3333", Role: "voter", Count: 1}, found: false, code: 404, }, @@ -217,7 +217,7 @@ func (suite *ruleTestSuite) TestGet() { url := fmt.Sprintf("%s/rule/%s/%s", suite.urlPrefix, testCase.rule.GroupID, testCase.rule.ID) if testCase.found { err = tu.ReadGetJSON(re, testDialClient, url, &resp) - suite.compareRule(&resp, &testCase.rule) + suite.compareRule(&resp, testCase.rule) } else { err = tu.CheckGetJSON(testDialClient, url, nil, tu.Status(re, testCase.code)) } diff --git a/tests/pdctl/region/region_test.go b/tests/pdctl/region/region_test.go index 1a5b5006499..5e381158672 100644 --- a/tests/pdctl/region/region_test.go +++ b/tests/pdctl/region/region_test.go @@ -213,7 +213,7 @@ func TestRegionNoLeader(t *testing.T) { re := require.New(t) ctx, cancel := context.WithCancel(context.Background()) defer cancel() - cluster, err := pdTests.NewTestCluster(ctx, 1) + cluster, err := tests.NewTestCluster(ctx, 1) re.NoError(err) err = cluster.RunInitialServers() re.NoError(err) @@ -237,10 +237,10 @@ func TestRegionNoLeader(t *testing.T) { }, } - leaderServer := cluster.GetLeaderServer() + leaderServer := cluster.GetServer(cluster.GetLeader()) re.NoError(leaderServer.BootstrapCluster()) for i := 0; i < len(stores); i++ { - pdTests.MustPutStore(re, cluster, stores[i]) + pdctl.MustPutStore(re, leaderServer.GetServer(), stores[i]) } metaRegion := &metapb.Region{ @@ -255,9 +255,9 @@ func TestRegionNoLeader(t *testing.T) { } r := core.NewRegionInfo(metaRegion, nil) - cluster.GetLeaderServer().GetRaftCluster().GetBasicCluster().SetRegion(r) + leaderServer.GetRaftCluster().GetBasicCluster().SetRegion(r) - cmd := ctl.GetRootCmd() - _, err = tests.ExecuteCommand(cmd, "-u", url, "region", "100") + cmd := pdctlCmd.GetRootCmd() + _, err = pdctl.ExecuteCommand(cmd, "-u", url, "region", "100") re.NoError(err) }