scheduler: refactor clusterInfo to extract a BasicCluster, and add mockCluster to easy testing #782

qqsun8819 · 2017-10-01T02:47:16Z

This pr resolves issue issue 752
1 add mockCluster in schedulers , and implements basic interface in schedule.Cluster.
2 move balance_test.go into schedulers
3 remove balance.go in server for duplicate code of schedulers/util.go

On the other hand, this pr imports some new bad smell code:
1 mockCluster has some duplicate code of clusterInfo in server/cache.go, due to dependency problem.
2 coordinator_test.go also some duplicate code of mockCluster, this is a remaining problem after we move balance_test.go out of server.
I plan to propose a new pr to resolve these two problems after this pr is merged

sre-bot · 2017-10-01T02:47:18Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

nolouch · 2017-10-04T08:06:37Z

server/schedulers/mockcluster.go

+
+// NewMockCluster creates a new mockCluster
+func newMockCluster(id *core.MockIDAllocator) *mockCluster {
+	return &mockCluster{


maybe we can let clusterinfo be public and use it directly

qqsun8819 · 2017-10-07T13:12:47Z

@disksing @nolouch We need to discuss about how we deal with clusterInfo in cache.go, as @nolouch proposed in his comment.
As I mentioned in pr description, now mockCluster has many duplicate code as clusterInfo, so we should extract clusterInfo along into a separate package; but as clusterInfo depends on many data structure in server package ( struct in replication.go etc.) , these module also has to be moved into separate package like core. This change is somewhat big...
According to these problems , I have some proposals we can discuss so we can proceed for further refactor:
1 move clusterInfo into "schedules" package, or into a new package like "cluster".
2 resolve all the dependency problems caused by the movement of clusterInfo in 1.
3 whether 1 and 2 should be done in a new pr after this pr is merged, or do it just in this pr.
As 1 and 2 caused many changes, what I prefer is open a new pr to do this, and I need your suggestion.

disksing · 2017-10-09T01:42:36Z

server/schedulers/balance_test.go

 	"github.com/pingcap/kvproto/pkg/pdpb"
 	"github.com/pingcap/pd/server/cache"
 	"github.com/pingcap/pd/server/core"
 	"github.com/pingcap/pd/server/schedule"
-	_ "github.com/pingcap/pd/server/schedulers" // Register schedulers for tests.
+	"math"


put it before 3rd-part libraries.

disksing · 2017-10-09T01:46:14Z

How about extract a cluster struct in core or scheduler contains following four fields:

stores          *core.StoresInfo
regions         *core.RegionsInfo
writeStatistics cache.Cache
readStatistics  cache.Cache

Then embed it into clusterInfo and mockClusterInfo.

@qqsun8819

siddontang · 2017-10-11T09:14:41Z

Any update @qqsun8819

qqsun8819 · 2017-10-14T04:52:20Z

@siddontang I'm working on it , plan to complete it this weekend according to @disksing 's suggestion

qqsun8819 · 2017-10-14T16:19:22Z

@disksing @nolouch @siddontang PR is updated according to review comments, PTAL if you have time

disksing · 2017-10-15T00:11:23Z

server/cache_test.go

@@ -337,22 +337,22 @@ func (s *testClusterInfoSuite) testRegionHeartbeat(c *C, cache *clusterInfo) {
 	n, np := uint64(3), uint64(3)

 	stores := newTestStores(3)
-	regions := newTestRegions(n, np)
+	Regions := newTestRegions(n, np)


Don't change local variable names...

disksing · 2017-10-15T00:12:03Z

server/cache_test.go

@@ -84,10 +84,10 @@ var _ = Suite(&testRegionsInfoSuite{})

 type testRegionsInfoSuite struct{}

-// Create n regions (0..n) of n stores (0..n).
+// Create n Regions (0..n) of n stores (0..n).


Don't need to change the comment.

disksing · 2017-10-15T00:13:32Z

server/core/test_util.go

+	return &MockIDAllocator{base: 0}
+}
+
+// Alloc return a new id


Alloc returns a new id.

disksing · 2017-10-15T00:15:47Z

server/schedulers/balance_test.go

 	"github.com/pingcap/kvproto/pkg/pdpb"
 	"github.com/pingcap/pd/server/cache"
 	"github.com/pingcap/pd/server/core"
 	"github.com/pingcap/pd/server/namespace"
 	"github.com/pingcap/pd/server/schedule"
-	_ "github.com/pingcap/pd/server/schedulers" // Register schedulers for tests.
+	"math"


We usually put builtin packages before 3rd-part ones.

siddontang · 2017-10-15T07:15:47Z

server/cache.go

@@ -36,25 +36,19 @@ var (
 )

 type clusterInfo struct {
+	*schedule.BasicCluster
 	sync.RWMutex


The RWMutex should also wrap the basic cluster, if yes, move the RWMutex to the first line of the clusterInfo.

siddontang · 2017-10-15T07:16:32Z

server/cache.go

 		return nil, errors.Trace(err)
 	}
-	log.Infof("load %v stores cost %v", c.stores.GetStoreCount(), time.Since(start))
+	log.Infof("load %v Stores cost %v", c.Stores.GetStoreCount(), time.Since(start))


using stores and regions in the log and comment is ok, no need to change them.

siddontang · 2017-10-15T07:17:00Z

server/cache_test.go

 	}
 }

 func (s *testClusterInfoSuite) testStoreHeartbeat(c *C, cache *clusterInfo) {
 	n, np := uint64(3), uint64(3)
 	stores := newTestStores(n)
-	regions := newTestRegions(n, np)
+	Regions := newTestRegions(n, np)


regions :=

disksing · 2017-10-15T15:24:21Z

LGTM.

nolouch

LGTM

disksing · 2017-10-17T05:18:12Z

/run-all-tests

qqsun8819 requested review from siddontang, nolouch and disksing October 1, 2017 02:47

nolouch reviewed Oct 4, 2017

View reviewed changes

disksing reviewed Oct 9, 2017

View reviewed changes

siddontang changed the title ~~scheduler: add mockCluster to ease scheduler testing~~ [WIP] scheduler: add mockCluster to ease scheduler testing Oct 13, 2017

qqsun8819 force-pushed the qqsun8819/mockcluster branch from 1f04348 to 1bc2717 Compare October 14, 2017 16:05

qqsun8819 changed the title ~~[WIP] scheduler: add mockCluster to ease scheduler testing~~ scheduler: add mockCluster to ease scheduler testing Oct 14, 2017

qqsun8819 added 5 commits October 15, 2017 00:06

add mockCluster

e36d925

fix make check problem

66343c4

merge balance_test.go with master

4a0eeba

extract basic cluster info into BasicCluster

6461fa1

merge with master for cache.go

4b7d001

qqsun8819 changed the title ~~scheduler: add mockCluster to ease scheduler testing~~ scheduler: refactor clusterInfo to extract a BasicCluster, and add mockCluster to easy testing Oct 14, 2017

qqsun8819 force-pushed the qqsun8819/mockcluster branch from 1bc2717 to 4b7d001 Compare October 14, 2017 16:17

disksing reviewed Oct 15, 2017

View reviewed changes

siddontang reviewed Oct 15, 2017

View reviewed changes

fix according to review

d38cc79

nolouch approved these changes Oct 16, 2017

View reviewed changes

Merge branch 'master' into qqsun8819/mockcluster

2df40e5

disksing merged commit efc52b5 into master Oct 17, 2017

disksing deleted the qqsun8819/mockcluster branch October 17, 2017 05:34

sre-bot added the contribution This PR is from a community contributor. label Dec 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: refactor clusterInfo to extract a BasicCluster, and add mockCluster to easy testing #782

scheduler: refactor clusterInfo to extract a BasicCluster, and add mockCluster to easy testing #782

qqsun8819 commented Oct 1, 2017

sre-bot commented Oct 1, 2017

nolouch Oct 4, 2017

qqsun8819 commented Oct 7, 2017

disksing Oct 9, 2017

disksing commented Oct 9, 2017

siddontang commented Oct 11, 2017

qqsun8819 commented Oct 14, 2017

qqsun8819 commented Oct 14, 2017

disksing Oct 15, 2017

qqsun8819 Oct 15, 2017

disksing Oct 15, 2017

qqsun8819 Oct 15, 2017

disksing Oct 15, 2017

disksing Oct 15, 2017

qqsun8819 Oct 15, 2017

siddontang Oct 15, 2017

siddontang Oct 15, 2017

qqsun8819 Oct 15, 2017

siddontang Oct 15, 2017

qqsun8819 Oct 15, 2017

disksing commented Oct 15, 2017

nolouch left a comment

disksing commented Oct 17, 2017

scheduler: refactor clusterInfo to extract a BasicCluster, and add mockCluster to easy testing #782

scheduler: refactor clusterInfo to extract a BasicCluster, and add mockCluster to easy testing #782

Conversation

qqsun8819 commented Oct 1, 2017

sre-bot commented Oct 1, 2017

Choose a reason for hiding this comment

qqsun8819 commented Oct 7, 2017

Choose a reason for hiding this comment

disksing commented Oct 9, 2017

siddontang commented Oct 11, 2017

qqsun8819 commented Oct 14, 2017

qqsun8819 commented Oct 14, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

disksing commented Oct 15, 2017

nolouch left a comment

Choose a reason for hiding this comment

disksing commented Oct 17, 2017