feature: define the interface SupernodeLocator #1294
Conversation
lowzj
force-pushed
the
SupernodeLocate
branch
2 times, most recently
from
8d4e4e0 to
3194dfe
Compare
Codecov Report
@@ Coverage Diff @@
## master #1294 +/- ##
==========================================
+ Coverage 51.16% 51.41% +0.25%
==========================================
Files 123 125 +2
Lines 8177 8240 +63
==========================================
+ Hits 4184 4237 +53
- Misses 3654 3660 +6
- Partials 339 343 +4
Continue to review full report at Codecov.
|
lowzj
force-pushed
the
SupernodeLocate
branch
3 times, most recently
from
5e6671a to
0520819
Compare
dfget/locator/locator.go
Outdated
| Infos map[string]string | ||
| } | ||
|
|
||
| // Supernode basic information of supernodes. |
There was a problem hiding this comment.
| // Supernode basic information of supernodes. | |
| // Supernode holds the basic information of supernodes. |
dfget/locator/locator.go
Outdated
| Metrics *SupernodeMetrics | ||
| } | ||
|
|
||
| // SupernodeMetrics the metrics used for locator to choose supernode. |
There was a problem hiding this comment.
| // SupernodeMetrics the metrics used for locator to choose supernode. | |
| // SupernodeMetrics holds metrics used for the locator to choose supernode. |
| ) | ||
|
|
||
| func init() { | ||
| rand.Seed(time.Now().UnixNano()) |
There was a problem hiding this comment.
Maybe we can consider moving rand.Seed(time.Now().UnixNano()) to the beginning of the main function to ensure that it always work and also avoids calling it everywhere. 😕
func main() {
rand.Seed(time.Now().UnixNano())
app.Execute()
}There was a problem hiding this comment.
How about move rand.Seed to the top package of dfget rather than main function? I think this operation should be transparent to the caller.
There was a problem hiding this comment.
Sure. That's OK with me. Just don't like calling rand.Seed everywhere!
There was a problem hiding this comment.
In order to ensure the independence of this pr, I open another pr #1318 to do this, please take a look.
| return | ||
| } | ||
|
|
||
| func (s *StaticLocator) Refresh() bool { |
There was a problem hiding this comment.
This looks more like Reset 😅 How about rename as Reset.
There was a problem hiding this comment.
This implementation looks more like Reset. But considering the different behaviors of other implementations, it may get a new list of supernodes via invoking this method.
So IMOP, Reresh is more appropriate. WDYT?
dfget/locator/static_locator.go
Outdated
| // configuration or CLI. | ||
| func NewStaticLocator(nodes []*config.NodeWeight) *StaticLocator { | ||
| locator := &StaticLocator{} | ||
| if nodes == nil || len(nodes) == 0 { |
There was a problem hiding this comment.
Using len(nodes) == 0 is enough here.
dfget/locator/static_locator.go
Outdated
| if idx >= len(s.Group.Nodes) { | ||
| return nil | ||
| } | ||
| return s.Group.Nodes[idx] |
There was a problem hiding this comment.
How about extract a common method to get node with specified index?
func (s *StaticLocator) getNode(index int32) *Supernode(){
if s.Group == nil || index >= len(s.Group.Nodes) {
return nil
}
return s.Group.Nodes[idx]
}
| Next() *Supernode | ||
|
|
||
| // GetGroup returns the group. | ||
| GetGroup(name string) *SupernodeGroup |
There was a problem hiding this comment.
Just out of curiosity, why consider add a supernode group concept here? And the SupernodeGroup can only be retrieved for viewing. Is it expected that I can choose a group to use?
There was a problem hiding this comment.
For example, we can group supernodes by region, business, version and so on. The implementation of SupernodeLocator can select a supernode based on the group. The caller does not need to care about the specific selection logic, but if it is really needed, the supernode list can also be obtained through this interface.
|
Please make a rebase for this PR. 😄 |
lowzj
force-pushed
the
SupernodeLocate
branch
3 times, most recently
from
a7d3a4d to
0491012
Compare
Signed-off-by: lowzj <[email protected]>
|
LGTM. |
Signed-off-by: Gaius <[email protected]>
Signed-off-by: lowzj [email protected]
Ⅰ. Describe what this PR did
issues: #1293
This pull request just defines the
SupernodeLocatorinterface, I will open another pull request to refactor the dfclient to use this interface to get supernode.Ⅱ. Does this pull request fix one issue?
Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews