refactor(cluster): refactor cluster package #1507

justxuewei · 2021-10-09T07:34:07Z

What this PR does:

The new directory structure is shown at below.

├── cluster              // abstract cluster
│   ├── cluster          // cluster interface & implementation
│   │   ├── available    // available cluster implementation
│   │   │   ├── ...
│   │   ├── base         // base cluster implementation
│   │   │   ├── ...
│   │   ├── broadcast    // broadcast cluster implementation
│   │   │   ├── ...
│   │   ├── failback     // failback cluster implementation
│   │   │   ├── ...
│   │   ├── failfast     // failfast cluster implementation
│   │   │   ├── ...
│   │   ├── failover     // failover cluster implementation
│   │   │   ├── ...
│   │   ├── failsafe     // failsafe cluster implementation
│   │   │   ├── ...
│   │   ├── forking      // forking cluster implementation
│   │   │   ├── ...
│   │   └── zoneaware    // zoneaware cluster implementation
│   │       ├── ...
│   ├── cluster_impl     // compatible with legacy imports, will be REMOVED in the future
│   │   └── import.go
│   ├── directory        // directory interface & implementation
│   │   ├── base
│   │   │   ├── ...
│   │   └── static
│   │       ├── ...
│   ├── loadbalance      // load balance interface & implementation
│   │   ├── consistenthashing   // consistenthashing load balance implementation
│   │   │   ├── ...
│   │   ├── leastactive  // leastactive load balance implementation
│   │   │   ├── ...
│   │   ├── random       // random load balance implementation
│   │   │   ├── ...
│   │   ├── roundrobin   // roundrobin load balance implementation
│   │   │   ├── ...
│   └── router           // NOT modified in this PR
│       ├── ...

PS: This refactor will be transparent for Dubbo-Go users.

Which issue(s) this PR fixes:

Fixes #1503

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Please note that importing "_ dubbo.apache.org/dubbo-go/v3/cluster/loadbalance" for loadbalance initialization couldn't work anymore.

Users must import used loadbalances respectively, or import `_ dubbo.apache.org/dubbo-go/v3/imports`(highly recommended way) instead.

Here are an example of importing all of loadbalances respectively:
import (
    _ "dubbo.apache.org/dubbo-go/v3/cluster/loadbalance/consistenthashing"
    _ "dubbo.apache.org/dubbo-go/v3/cluster/loadbalance/leastactive"
    _ "dubbo.apache.org/dubbo-go/v3/cluster/loadbalance/random"
    _ "dubbo.apache.org/dubbo-go/v3/cluster/loadbalance/roundrobin"
)

codecov-commenter · 2021-10-09T08:02:22Z

Codecov Report

Merging #1507 (816594a) into 3.0 (f8bdd45) will decrease coverage by 0.17%.
The diff coverage is 77.87%.

@@            Coverage Diff             @@
##              3.0    #1507      +/-   ##
==========================================
- Coverage   42.03%   41.86%   -0.18%     
==========================================
  Files         262      259       -3     
  Lines       15171    15109      -62     
==========================================
- Hits         6377     6325      -52     
+ Misses       8054     8049       -5     
+ Partials      740      735       -5

Impacted Files	Coverage Δ
common/extension/cluster.go	`0.00% <ø> (ø)`
common/extension/loadbalance.go	`0.00% <0.00%> (ø)`
common/extension/registry_directory.go	`0.00% <0.00%> (ø)`
config/reference_config.go	`0.00% <0.00%> (ø)`
cluster/cluster/base/cluster_invoker.go	`24.46% <23.33%> (ø)`
cluster/cluster/forking/cluster_invoker.go	`58.33% <55.55%> (ø)`
cluster/directory/base/directory.go	`45.71% <55.55%> (ø)`
registry/directory/directory.go	`76.42% <71.42%> (ø)`
cluster/cluster/failover/cluster_invoker.go	`66.66% <75.00%> (ø)`
...uster/loadbalance/consistenthashing/loadbalance.go	`89.47% <89.47%> (ø)`
... and 26 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 f8bdd45...816594a. Read the comment docs.

Mulavar · 2021-10-09T08:19:23Z

有两个小疑问：

看实现是把 cluster 结构体的语义部分都提取到了包名中，结构体命名只保留最基本的部分，比如 failbackClusterInvoker 就把 failback 提取到包路径上，结构体只保留后面的 clusterInvoker 部分，这个方式的可读性是不是更好呢？我举个例子，比如你用 goland 做跳转实现的时候，之前看到的是一堆 xxxClusterInvoke，现在看到的都是同名的结构体，需要进一步看路径名判断跳转，这样会不会反而增加了使用时的可读门槛？
常量命名要统一改驼峰形式么，可以定下来这个问题，后面统一调整一下？

justxuewei · 2021-10-09T08:25:48Z

这样会不会反而增加了使用时的可读门槛？

我个人认为不会，因为对外访问一般是通过 extension.GetCluster 函数，该函数返回的是一个 cluster.Cluster 的 interface，用户需要通过 interface 再找到相应的 cluster 定义，一般不会直接使用 xxxCluster（Invoker 同理，一般都会先定位到 interface）。在现有目录下，每种 cluster 的结构和定义都更加清晰，且有更良好的扩充性，这是我想要做这次重构的初衷。

常量命名要统一改驼峰形式

是的，Golang 中 XX_YY_ZZ 本身是不符合规范的，所以驼峰式命名我个人认为会好一些。但是本次 PR 只是重构 cluster 相关代码，不涉及到常量名重构，这部分内容可以以后在实现。

Mulavar · 2021-10-09T08:32:06Z

这样会不会反而增加了使用时的可读门槛？

我个人认为不会，因为对外访问一般是通过 extension.GetCluster 函数，该函数返回的是一个 cluster.Cluster 的 interface，用户需要通过 interface 再找到相应的 cluster 定义，一般不会直接使用 xxxCluster（Invoker 同理，一般都会先定位到 interface）。在现有目录下，每种 cluster 的结构和定义都更加清晰，且有更良好的扩充性，这是我想要做这次重构的初衷。

常量命名要统一改驼峰形式

是的，Golang 中 XX_YY_ZZ 本身是不符合规范的，所以驼峰式命名我个人认为会好一些。但是本次 PR 只是重构 cluster 相关代码，不涉及到常量名重构，这部分内容可以以后在实现。

第一个我指的场景就是指 interface 跳转到实现的时候，之前跳转根据结构体命名可以直接找对应的，但现在结构体命名都一样了，他就需要根据你的路径去区分，我理解你做这个思路，拆分目录肯定是更有利的，我的疑问可能更集中在所有 cluster 和 invoker 的命名被简化了这一点上（语义被移到了包路径上，这是一个有点奇怪的事）。
第二个我建议要不在社区里公告一下，确定我们之后一定会统一成驼峰形式，这样也可以避免万一意见不统一又回过头来把这个pr的内容改回去了。

justxuewei · 2021-10-09T08:35:20Z

@Mulavar 现在确实有这个问题，但是 GoLand 里有路径提示，我觉得可能会有所缓解。

Mulavar · 2021-10-09T08:38:47Z

@Mulavar 现在确实有这个问题，但是 GoLand 里有路径提示，我觉得可能会有所缓解。

是的，goland 是有其他的方式可以缓解，只是这种方式相比之前没那么直观。或者反过来看，为什么我们要把这部分语义放到路径中要去掉呢，我认为即使保留在结构体命名中也不影响代码的简洁和精炼？

justxuewei · 2021-10-09T08:43:18Z

@Mulavar Ref: https://go.dev/blog/package-names

Avoid repetition. Since client code uses the package name as a prefix when referring to the package contents, the names for those contents need not repeat the package name. The HTTP server provided by the http package is called Server, not HTTPServer. Client code refers to this type as http.Server, so there is no ambiguity.

cluster/loadbalance/consistenthashing/loadbalance.go

config/reference_config.go

justxuewei added 8 commits October 7, 2021 21:25

refactor(cluster): refactor cluster/cluster

df86b3c

refactor(cluster): refactor directory and loadbalance

b43028b

fix(cluster): fix import cycles

79252b8

style(cluster): go fmt

763253a

style(cluster): format code by 3-block style

e8d682b

style(cluster): go fmt

f6ccdbe

fix(cluster): fix key bugs

a9dea0f

fix(cluster): fix apache license

0b746ac

justxuewei changed the title ~~WIP: refactor(cluster): refactor cluster package~~ refactor(cluster): refactor cluster package Oct 9, 2021

Mulavar approved these changes Oct 9, 2021

View reviewed changes

AlexStocks reviewed Oct 9, 2021

View reviewed changes

cluster/loadbalance/consistenthashing/loadbalance.go Outdated Show resolved Hide resolved

AlexStocks reviewed Oct 9, 2021

View reviewed changes

config/reference_config.go Outdated Show resolved Hide resolved

fix(cluster): fix constant name

a8f2081

justxuewei requested a review from AlexStocks October 10, 2021 04:20

style(cluster): go fmt

816594a

AlexStocks approved these changes Oct 10, 2021

View reviewed changes

LaurenceLiZhixin approved these changes Oct 11, 2021

View reviewed changes

LaurenceLiZhixin merged commit e213f97 into apache:3.0 Oct 11, 2021

justxuewei deleted the refactor/cluster branch December 14, 2021 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cluster): refactor cluster package #1507

refactor(cluster): refactor cluster package #1507

justxuewei commented Oct 9, 2021

codecov-commenter commented Oct 9, 2021 •

edited

Loading

Mulavar commented Oct 9, 2021

justxuewei commented Oct 9, 2021 •

edited

Loading

Mulavar commented Oct 9, 2021

justxuewei commented Oct 9, 2021

Mulavar commented Oct 9, 2021

justxuewei commented Oct 9, 2021

refactor(cluster): refactor cluster package #1507

refactor(cluster): refactor cluster package #1507

Conversation

justxuewei commented Oct 9, 2021

codecov-commenter commented Oct 9, 2021 • edited Loading

Codecov Report

Mulavar commented Oct 9, 2021

justxuewei commented Oct 9, 2021 • edited Loading

Mulavar commented Oct 9, 2021

justxuewei commented Oct 9, 2021

Mulavar commented Oct 9, 2021

justxuewei commented Oct 9, 2021

codecov-commenter commented Oct 9, 2021 •

edited

Loading

justxuewei commented Oct 9, 2021 •

edited

Loading