Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

optimize: refactor loadBalance config from registry.conf to file.conf. #3248

Merged
merged 18 commits into from
Mar 19, 2021

Conversation

ls9527
Copy link
Contributor

@ls9527 ls9527 commented Nov 1, 2020

Ⅰ. Describe what this PR did

loadbalance 的配置在 registry.conf 文件, 这个文件属于配置中心的基础配置, 除了指定方式和基础信息以外,其他信息不应该在这个文件。 把 loadBalance 改到核心配置文件中(file.conf).

例如随机负载均衡

loadBalance = {
    type = "RandomLoadBalance"
}

还有一致性hash负载均衡

loadBalance = {
    type = "ConsistentHashLoadBalance"
    visualNodes=10
}

Ⅱ. 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

@caohdgege
Copy link
Contributor

caohdgege commented Nov 1, 2020

[ERROR] /home/travis/build/seata/seata/seata-spring-boot-starter/src/test/java/io/seata/spring/boot/autoconfigure/PropertiesTest.java:[223,59] cannot find symbol
  symbol:   class LoadBalanceProperties
  location: class io.seata.spring.boot.autoconfigure.PropertiesTest
[ERROR] /home/travis/build/seata/seata/seata-spring-boot-starter/src/test/java/io/seata/spring/boot/autoconfigure/PropertiesTest.java:[224,42] cannot find symbol
  symbol:   class LoadBalanceProperties
  location: class io.seata.spring.boot.autoconfigure.PropertiesTest

@caohdgege
Copy link
Contributor

处理一下ci的报错

Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition, please in server/file. Conf. Example, script/server/conf and client/conf&spring, add configuration information

import io.seata.spring.boot.autoconfigure.properties.registry.RegistryRedisProperties;
import io.seata.spring.boot.autoconfigure.properties.registry.RegistrySofaProperties;
import io.seata.spring.boot.autoconfigure.properties.registry.RegistryZooKeeperProperties;
import io.seata.spring.boot.autoconfigure.properties.registry.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • is not allowed

@@ -1,8 +1,7 @@
registry {
# file 、nacos 、eureka、redis、zk、consul、etcd3、sofa
type = "file"
loadBalance = "RandomLoadBalance"
loadBalanceVirtualNodes = 10

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank lines can be removed

@funky-eyes
Copy link
Contributor

@ph3636 PTAL

@codecov-io
Copy link

codecov-io commented Nov 2, 2020

Codecov Report

Merging #3248 (f977d55) into develop (ded2c83) will decrease coverage by 0.00%.
The diff coverage is 61.53%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #3248      +/-   ##
=============================================
- Coverage      51.54%   51.53%   -0.01%     
+ Complexity      3376     3375       -1     
=============================================
  Files            617      618       +1     
  Lines          20474    20477       +3     
  Branches        2567     2567              
=============================================
  Hits           10553    10553              
- Misses          8861     8862       +1     
- Partials        1060     1062       +2     
Impacted Files Coverage Δ Complexity Δ
...ing/boot/autoconfigure/SeataAutoConfiguration.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ta/spring/boot/autoconfigure/StarterConstants.java 100.00% <ø> (ø) 1.00 <0.00> (ø)
...figure/properties/registry/RegistryProperties.java 60.00% <ø> (+6.15%) 2.00 <0.00> (-2.00) ⬆️
...ure/properties/registry/LoadBalanceProperties.java 55.55% <55.55%> (ø) 3.00 <3.00> (?)
...scovery/loadbalance/ConsistentHashLoadBalance.java 89.28% <100.00%> (ø) 3.00 <1.00> (ø)
...eata/discovery/loadbalance/LoadBalanceFactory.java 66.66% <100.00%> (+16.66%) 1.00 <1.00> (ø)
...o/seata/server/coordinator/DefaultCoordinator.java 53.92% <0.00%> (-0.50%) 28.00% <0.00%> (-1.00%)
...in/java/io/seata/server/session/GlobalSession.java 83.63% <0.00%> (-0.46%) 71.00% <0.00%> (-1.00%)

@ph3636
Copy link
Contributor

ph3636 commented Nov 2, 2020

Search it according to the keyword (loadBalanceVirtualNodes), or check it through git records, there are still many places that have not been changed.
根据关键词(loadBalanceVirtualNodes)搜一下,或者通过git记录看下,还有好多地方没改。

@ls9527
Copy link
Contributor Author

ls9527 commented Nov 2, 2020

@ph3636 好的, 修改了。
@caohdgege 已经提交了遗漏提交的文件

@ph3636
Copy link
Contributor

ph3636 commented Nov 3, 2020

I think this configuration is more appropriate to put in the client, the server will not use this configuration
我觉得这个配置放到client中比较合适,server不会用到这个配置

@ls9527
Copy link
Contributor Author

ls9527 commented Nov 3, 2020

I think this configuration is more appropriate to put in the client, the server will not use this configuration
我觉得这个配置放到client中比较合适,server不会用到这个配置

有道理阿, 只有TM,RM才需要负载均衡。 TC都知道发给谁了,就不用均衡了。 那我在改改

@ph3636
Copy link
Contributor

ph3636 commented Nov 4, 2020

Change these files
改一下这些文件
script/client/spring/application.properties
script/client/spring/application.yml
META-INF/additional-spring-configuration-metadata.json

No configuration in these files
下面这些文件中可以不加配置
script/server/config

There are some other details, such as wrong name, extra blank lines, etc.
还有一些其他的细节,比如名字错误,多余空行等等

private static final String VIRTUAL_NODES = FILE_ROOT_REGISTRY + FILE_CONFIG_SPLIT_CHAR + "loadBalanceVirtualNodes";
private static final int VIRTUAL_NODES_NUM = ConfigurationFactory.CURRENT_FILE_INSTANCE.getInt(VIRTUAL_NODES, VIRTUAL_NODES_DEFAULT);
/**
* The constant LOAD_BALANCE_TYPE.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this name
改一下这个名字

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the name or delete it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

一直以为改了,今天核对才发现没改。 刚刚改掉了

@@ -25,3 +25,5 @@ store.redis.database=0
store.redis.minConn=1
store.redis.maxConn=10
store.redis.queryLimit=100
loadBalance.type=RandomLoadBalance
loadBalance.consistentHash.virtualNodes=10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are redundant
这些是多余

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

多余的删掉了

loadBalance:
type: RandomLoadBalance
consistentHash:
virtualNodes: 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are redundant
这些是多余

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

多谢大神, 已将多余的部分删除。 将loadBalance的 type 和 visualNodes 区分开了。

Copy link
Contributor

@ph3636 ph3636 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

private static final String VIRTUAL_NODES = FILE_ROOT_REGISTRY + FILE_CONFIG_SPLIT_CHAR + "loadBalanceVirtualNodes";
private static final int VIRTUAL_NODES_NUM = ConfigurationFactory.CURRENT_FILE_INSTANCE.getInt(VIRTUAL_NODES, VIRTUAL_NODES_DEFAULT);
/**
* The constant LOAD_BALANCE_TYPE.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the name or delete it

script/server/config/file.conf Outdated Show resolved Hide resolved
script/server/config/file.properties Outdated Show resolved Hide resolved
script/server/config/file.yml Outdated Show resolved Hide resolved
server/src/main/resources/file.conf Outdated Show resolved Hide resolved
@funky-eyes
Copy link
Contributor

请解决代码冲突

@funky-eyes funky-eyes added this to the 1.5.0 milestone Nov 30, 2020
@funky-eyes funky-eyes added module/config config module module/discovery discovery module labels Nov 30, 2020
…balance_1101

# Conflicts:
#	discovery/seata-discovery-core/src/main/java/io/seata/discovery/loadbalance/ConsistentHashLoadBalance.java
#	script/client/conf/file.conf
#	script/client/spring/application.properties
#	script/client/spring/application.yml
#	seata-spring-boot-starter/src/main/java/io/seata/spring/boot/autoconfigure/SeataAutoConfiguration.java
#	seata-spring-boot-starter/src/main/java/io/seata/spring/boot/autoconfigure/StarterConstants.java
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -59,7 +59,7 @@
- [[#3420](https://github.com/seata/seata/pull/3420)] optimize enumerated classes and add unit tests
- [[#3436](https://github.com/seata/seata/pull/3436)] optimize typo in SQLType class
- [[#3439](https://github.com/seata/seata/pull/3439)] adjust the order of springApplicationContextProvider

- [[#3248](https://github.com/seata/seata/pull/3248)] optimize the config of load-balance migration to belong the client node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

migrate instead of migration, migration is not a verb.

Copy link
Contributor

@l81893521 l81893521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xingfudeshi xingfudeshi self-requested a review March 19, 2021 02:48
Copy link
Member

@xingfudeshi xingfudeshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@xingfudeshi xingfudeshi merged commit d28274d into apache:develop Mar 19, 2021
/**
* The constant LOAD_BALANCE_CONSISTENT_HASH_VISUAL_NODES.
*/
public static final String LOAD_BALANCE_CONSISTENT_HASH_VISUAL_NODES = LOAD_BALANCE_PREFIX + "visualNodes";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xingfudeshi
配置名 visualNodesLoadBalanceProperties 类里的field virtualNodes 名不一致,导致在 SpringBootConfigurationProvider 下,获取字段时不存在,导致返回配置值为 null
也就是说在springboot下,配置了seata.client.load-balance.visual-nodesseata.client.load-balance.virtual-nodes都会导致配置读取不到。

public class SpringBootConfigurationProvider implements ExtConfigurationProvider {

    ....

    private Object getFieldValue(Object object, String fieldName, String dataId) throws IllegalAccessException {
        Optional<Field> fieldOptional = Stream.of(object.getClass().getDeclaredFields())
            .filter(f -> f.getName().equalsIgnoreCase(fieldName)).findAny();
        if (fieldOptional.isPresent()) {
            Field field = fieldOptional.get();
            if (Objects.equals(field.getType(), Map.class)) {
                return getConfig(dataId, null, String.class);
            }
            field.setAccessible(true);
            Object defaultValue = field.get(object);
            return getConfig(dataId, defaultValue, field.getType());
        }
        return null;
    }

    ....

}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/config config module module/discovery discovery module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants