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

checker, statistic: avoid leak in label statistic #8703

Merged
merged 5 commits into from
Oct 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func HandleOverlaps(ctx context.Context, c Cluster, overlaps []*core.RegionInfo)
if c.GetRegionStats() != nil {
c.GetRegionStats().ClearDefunctRegion(item.GetID())
}
c.GetLabelStats().ClearDefunctRegion(item.GetID())
c.GetLabelStats().MarkDefunctRegion(item.GetID())
c.GetRuleManager().InvalidCache(item.GetID())
}
}
Expand Down
7 changes: 5 additions & 2 deletions pkg/mcs/scheduling/server/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,15 +379,18 @@ func (c *Cluster) waitSchedulersInitialized() {
}
}

// TODO: implement the following methods

// UpdateRegionsLabelLevelStats updates the status of the region label level by types.
func (c *Cluster) UpdateRegionsLabelLevelStats(regions []*core.RegionInfo) {
for _, region := range regions {
c.labelStats.Observe(region, c.getStoresWithoutLabelLocked(region, core.EngineKey, core.EngineTiFlash), c.persistConfig.GetLocationLabels())
}
}

// ClearDefunctRegionsLabelLevelStats clears the defunct regions' label level stats.
func (c *Cluster) ClearDefunctRegionsLabelLevelStats() {
c.labelStats.ClearDefunctRegions()
}

func (c *Cluster) getStoresWithoutLabelLocked(region *core.RegionInfo, key, value string) []*core.StoreInfo {
stores := make([]*core.StoreInfo, 0, len(region.GetPeers()))
for _, p := range region.GetPeers() {
Expand Down
3 changes: 3 additions & 0 deletions pkg/mock/mockcluster/mockcluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@
// UpdateRegionsLabelLevelStats updates the label level stats for the regions.
func (*Cluster) UpdateRegionsLabelLevelStats(_ []*core.RegionInfo) {}

// ClearDefunctRegionsLabelLevelStats clears the defunct regions' label level stats.
func (*Cluster) ClearDefunctRegionsLabelLevelStats() {}

Check warning on line 129 in pkg/mock/mockcluster/mockcluster.go

View check run for this annotation

Codecov / codecov/patch

pkg/mock/mockcluster/mockcluster.go#L129

Added line #L129 was not covered by tests

// LoadRegion puts region info without leader
func (mc *Cluster) LoadRegion(regionID uint64, peerStoreIDs ...uint64) {
// regions load from etcd will have no leader
Expand Down
2 changes: 2 additions & 0 deletions pkg/schedule/checker/checker_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ func (c *Controller) PatrolRegions() {
c.cluster.UpdateRegionsLabelLevelStats(regions)
// When the key is nil, it means that the scan is finished.
if len(key) == 0 {
// Clear the defunct regions label level statistics.
c.cluster.ClearDefunctRegionsLabelLevelStats()
lhy1024 marked this conversation as resolved.
Show resolved Hide resolved
// update the scan limit.
c.patrolRegionScanLimit = calculateScanLimit(c.cluster)
// update the metrics.
Expand Down
1 change: 1 addition & 0 deletions pkg/schedule/core/cluster_informer.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ type CheckerCluster interface {
GetCheckerConfig() sc.CheckerConfigProvider
GetStoreConfig() sc.StoreConfigProvider
UpdateRegionsLabelLevelStats(regions []*core.RegionInfo)
ClearDefunctRegionsLabelLevelStats()
}

// SharedCluster is an aggregate interface that wraps multiple interfaces
Expand Down
24 changes: 19 additions & 5 deletions pkg/statistics/region_collection.go
Original file line number Diff line number Diff line change
Expand Up @@ -365,13 +365,15 @@ type LabelStatistics struct {
syncutil.RWMutex
regionLabelStats map[uint64]string
labelCounter map[string]int
defunctRegions map[uint64]struct{}
}

// NewLabelStatistics creates a new LabelStatistics.
func NewLabelStatistics() *LabelStatistics {
return &LabelStatistics{
regionLabelStats: make(map[uint64]string),
labelCounter: make(map[string]int),
defunctRegions: make(map[uint64]struct{}),
}
}

Expand Down Expand Up @@ -405,14 +407,26 @@ func ResetLabelStatsMetrics() {
regionLabelLevelGauge.Reset()
}

// ClearDefunctRegion is used to handle the overlap region.
func (l *LabelStatistics) ClearDefunctRegion(regionID uint64) {
// MarkDefunctRegion is used to handle the overlap region.
// It is used to mark the region as defunct and remove it from the label statistics later.
func (l *LabelStatistics) MarkDefunctRegion(regionID uint64) {
l.Lock()
defer l.Unlock()
if label, ok := l.regionLabelStats[regionID]; ok {
l.labelCounter[label]--
delete(l.regionLabelStats, regionID)
l.defunctRegions[regionID] = struct{}{}
}

// ClearDefunctRegions is used to handle the overlap region.
// It is used to remove the defunct regions from the label statistics.
func (l *LabelStatistics) ClearDefunctRegions() {
Copy link
Member

Choose a reason for hiding this comment

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

How about adding a unit test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I have added a unit test, which can pass in this pr and is failed in master branch.

l.Lock()
defer l.Unlock()
for regionID := range l.defunctRegions {
if label, ok := l.regionLabelStats[regionID]; ok {
l.labelCounter[label]--
delete(l.regionLabelStats, regionID)
}
}
l.defunctRegions = make(map[uint64]struct{})
}

// GetLabelCounter is only used for tests.
Expand Down
5 changes: 5 additions & 0 deletions server/cluster/scheduling_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,11 @@ func (sc *schedulingController) UpdateRegionsLabelLevelStats(regions []*core.Reg
}
}

// ClearDefunctRegionsLabelLevelStats clears the status of the region label level by types.
func (sc *schedulingController) ClearDefunctRegionsLabelLevelStats() {
sc.labelStats.ClearDefunctRegions()
}

func (sc *schedulingController) getStoresWithoutLabelLocked(region *core.RegionInfo, key, value string) []*core.StoreInfo {
stores := make([]*core.StoreInfo, 0, len(region.GetPeers()))
for _, p := range region.GetPeers() {
Expand Down