server/schedule: remove unnecessary interfaces

[disksing]

What problem does this PR solve?

Cleanup code, remove unnecessary interfaces.
Fix #1664

What is changed and how it works?

• remove cluster.AllocPeer(), use id.Allocator interface directly
• rename allocator.Alloc() to allocator.AllocID
• remove operator.Cluster, use smaller interfaces instead.

Check List

Tests

• Unit test

[codecov-io]

Codecov Report

Merging #1679 into master will increase coverage by 0.04%.
The diff coverage is 91.39%.

@@            Coverage Diff             @@
##           master    #1679      +/-   ##
==========================================
+ Coverage   76.52%   76.57%   +0.04%     
==========================================
  Files         157      157              
  Lines       15352    15341      -11     
==========================================
- Hits        11748    11747       -1     
+ Misses       2587     2583       -4     
+ Partials     1017     1011       -6

Impacted Files	Coverage Δ
server/schedule/scheduler.go	66.66% <ø> (ø)	⬆️
server/cluster_worker.go	47.89% <0%> (ø)	⬆️
server/schedulers/balance_region.go	86.74% <100%> (ø)	⬆️
server/grpc_service.go	59.3% <100%> (+0.21%)	⬆️
server/id/id.go	77.27% <100%> (ø)	⬆️
server/checker/namespace_checker.go	75.38% <100%> (ø)	⬆️
server/schedulers/shuffle_hot_region.go	60.52% <100%> (-6.58%)	⬇️
server/schedulers/hot_region.go	83.63% <100%> (ø)	⬆️
server/schedulers/random_merge.go	66.66% <100%> (+5.12%)	⬆️
pkg/mock/mockcluster/mockcluster.go	91.54% <100%> (-0.07%)	⬇️
... and 22 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 96e2588...68090d1. Read the comment docs.

[Luffbee]

Passing the cluster twice when calling a function is really weird. And separating the Cluster interface into super-interfaces in parameter list seems not a good idea: imagine that a function need all the super-interfaces, its parameter list will become very long.

[disksing]

@Luffbee The basic idea here is to avoid using complex all-in-one Cluster interface and reduce dependency. I don't think any functions in /operator will need all methods that are listed in the previous Cluster someday. By now, there are at most 2 parameters, it looks acceptable.

Regarding the passing cluster twice issue, IMO it is actually caused by another complex all-in-one Cluster in the scheduler package. If we can break it down to small interfaces later then cluster, cluster can be avoided.

[Luffbee]

@Luffbee The basic idea here is to avoid using complex all-in-one Cluster interface and reduce dependency. I don't think any functions in /operator will need all methods that are listed in the previous Cluster someday. By now, there are at most 2 parameters, it looks acceptable.

Regarding the passing cluster twice issue, IMO it is actually caused by another complex all-in-one Cluster in the scheduler package. If we can break it down to small interfaces later then cluster, cluster can be avoided.

I'm not clear about what the "reduce dependency" means, but I think using an all-in-one Cluster interface is the right way. The schedule package is a toolkit for schedulers. A scheduler's jobs are:

1. read the status of the cluster
2. evaluate the status
3. make decisions to improve the status

So, what a scheduler want is not several interfaces like core.RegionSetInformer, but an all-in-one Cluster which can give all information it want.

What's more, using an all-in-one interface here makes the functions signature more uniform, and it is more flexible: if a function is modified and add a dependency, the signature won't need to change.

[disksing]

reduce dependency means that the operator package does not need to have any dependency on core.RegionSetInformer, core.StoreSetController, statistics.RegionStatInformer, or other interfaces that may potentially become part of Cluster in the future.

Schedulers do need all methods of Cluster, we may discuss it later about whether it is resonable to use a big interface for it. But this PR mainly focuses on removing operator.Cluster, I guess it does not affect how a scheduler want to read cluster status at all.

PS. There is a Go Proverb says The bigger the interface, the weaker the abstraction, you may want to check here for some inspiration.

[shafreeck]

Why a cluster should be passed as an argument twice? Does this a mistake?

[shafreeck]

It is weird to pass m.cluster twice

[disksing]

@shafreeck Will use another approach. Closing.