server: remove extra region clone when handle region heartbeat #1195
Conversation
| @@ -446,7 +446,6 @@ func (c *clusterInfo) updateStoreStatusLocked(id uint64) { | |||
|
|
|||
| // handleRegionHeartbeat updates the region information. | |||
| func (c *clusterInfo) handleRegionHeartbeat(region *core.RegionInfo) error { | |||
| region = region.Clone() | |||
There was a problem hiding this comment.
The region is used after calling this function. Are you sure that it is safe to not clone? See: https://github.com/pingcap/pd/blob/9b0763fb0474c657c1abbef8238871f3d1284817/server/cluster_worker.go#L28-L41
There was a problem hiding this comment.
Actually, all function only read it. More reassuring, I clone the region which has operator now.
There was a problem hiding this comment.
It does not make sense to clone the region. Cloning the region reads the region too.
I'm not so sure that the region is inmutable. I think you can propose another PR to change all fields of core.RegionInfo to unexported fields and add read only methods to access them. After that we can be sure that the RegionInfo is inmutable and discard the Clone method of it.
|
/run-all-tests |
server/api/admin_test.go
Outdated
| @@ -71,7 +71,7 @@ func (s *testAdminSuite) TestDropRegion(c *C) { | |||
| c.Assert(err, IsNil) | |||
| c.Assert(res.StatusCode, Equals, http.StatusOK) | |||
| res.Body.Close() | |||
| err = cluster.HandleRegionHeartbeat(region) | |||
| err = cluster.HandleRegionHeartbeat(region.Clone()) | |||
There was a problem hiding this comment.
Can we remove the Clone method?
server/cluster_info_test.go
Outdated
| @@ -378,20 +378,12 @@ func (s *testClusterInfoSuite) TestRegionHeartbeat(c *C) { | |||
| checkRegionsKV(c, cluster.kv, regions[:i+1]) | |||
|
|
|||
| // region is updated. | |||
server/cluster_info_test.go
Outdated
| ) | ||
| regions[i] = region | ||
| c.Assert(cluster.handleRegionHeartbeat(region), IsNil) | ||
| checkRegions(c, cluster.core.Regions, regions[:i+1]) | ||
| checkRegionsKV(c, cluster.kv, regions[:i+1]) |
nolouch
force-pushed
the
remove-clone
branch
from
20d0b80 to
acf87a2
Compare
What have you changed? (required)
the region variable already is a new object from the request. we only read it in one goroutine now, so the clone is useless, remove it.
What are the type of the changes (required)?
How has this PR been tested (required)?
manual tests
unit tests