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

feature: support group configuration in nacos registry #2620

Merged
merged 38 commits into from
Jun 29, 2020
Merged

feature: support group configuration in nacos registry #2620

merged 38 commits into from
Jun 29, 2020

Conversation

hsoftxl
Copy link
Contributor

@hsoftxl hsoftxl commented Apr 27, 2020

发现TC服务支持 配置Group

@codecov-io
Copy link

codecov-io commented Apr 27, 2020

Codecov Report

Merging #2620 into develop will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2620      +/-   ##
=============================================
- Coverage      50.81%   50.80%   -0.01%     
+ Complexity      2814     2812       -2     
=============================================
  Files            558      558              
  Lines          17931    17931              
  Branches        2126     2124       -2     
=============================================
- Hits            9111     9110       -1     
- Misses          7953     7954       +1     
  Partials         867      867              
Impacted Files Coverage Δ Complexity Δ
...e/properties/registry/RegistryNacosProperties.java 51.72% <50.00%> (-0.28%) 8.00 <1.00> (+1.00) ⬇️
...o/seata/server/coordinator/DefaultCoordinator.java 54.63% <0.00%> (-0.52%) 28.00% <0.00%> (-1.00%)
...in/java/io/seata/server/session/GlobalSession.java 83.71% <0.00%> (-0.46%) 71.00% <0.00%> (-1.00%)
...n/java/io/seata/rm/datasource/DataSourceProxy.java 37.73% <0.00%> (+0.89%) 10.00% <0.00%> (-1.00%) ⬆️

@l81893521 l81893521 added the first-time contributor first-time contributor label Apr 27, 2020
@funky-eyes
Copy link
Contributor

please use English for the title

@l81893521 l81893521 changed the title 发现TC服务支持 配置Group feature: support group configuration in nacos registry Apr 27, 2020
@hsoftxl hsoftxl changed the title feature: support group configuration in nacos registry support TC service configuration By Group in nacos Apr 27, 2020
@hsoftxl hsoftxl changed the title support TC service configuration By Group in nacos support TC service configuration By group property in nacos type Apr 27, 2020
@hsoftxl hsoftxl changed the title support TC service configuration By group property in nacos type support group configuration in nacos registry Apr 27, 2020
@l81893521
Copy link
Contributor

Please solve the conflict.

@funky-eyes
Copy link
Contributor

模仿一下其他读取key,跟默认值的做法,不要直接从nacos拿key,而要作为一个开放出去的key,config跟registry下的nacos也要进行添加对应的配置.比如

      nacos:
         application: seata-server
         serverAddr: 127.0.0.1:8848
         namespace:
         username: ""
         password: ""

你需要把你修改的group补充进去

@l81893521
Copy link
Contributor

Please change the title begin with "feature: ".

@jsbxyyx jsbxyyx changed the title support group configuration in nacos registry feature: support group configuration in nacos registry Apr 27, 2020
@zjinlei zjinlei added this to the 1.3.0 milestone May 2, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 20, 2020

Codecov Report

Merging #2620 into develop will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #2620      +/-   ##
=============================================
+ Coverage      50.14%   50.15%   +0.01%     
- Complexity      2951     2956       +5     
=============================================
  Files            591      591              
  Lines          18950    18954       +4     
  Branches        2285     2284       -1     
=============================================
+ Hits            9502     9506       +4     
- Misses          8503     8505       +2     
+ Partials         945      943       -2     
Impacted Files Coverage Δ Complexity Δ
...e/properties/registry/RegistryNacosProperties.java 51.72% <50.00%> (-0.28%) 8.00 <1.00> (+1.00) ⬇️
...in/java/io/seata/server/session/GlobalSession.java 84.16% <0.00%> (+0.45%) 72.00% <0.00%> (+1.00%)
...o/seata/server/coordinator/DefaultCoordinator.java 55.15% <0.00%> (+0.51%) 29.00% <0.00%> (+1.00%)

@l81893521 l81893521 added module/discovery discovery module module/seata-spring-boot-starter seata-spring-boot-starter module labels May 20, 2020
@funky-eyes
Copy link
Contributor

funky-eyes commented May 21, 2020

image

你确定测试了这个pr吗?我的服务分组名称还是DEFAULT_GROUP

@funky-eyes
Copy link
Contributor

register&unregister&subscribe Instance missing service group name

@hsoftxl
Copy link
Contributor Author

hsoftxl commented May 25, 2020

已修改

@@ -5,7 +5,8 @@ registry {
nacos {
application = "seata-server"
serverAddr = "127.0.0.1:8848"
namespace = ""
group = "SEATA_GROUP"
namespace = "publiic"
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong word spell. please fix.

@@ -58,7 +59,7 @@ config {

nacos {
serverAddr = "127.0.0.1:8848"
namespace = ""
namespace = "public"
Copy link
Contributor

Choose a reason for hiding this comment

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

please reset 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.

set namespace "" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

set namespace "" ?

不要去改动namespace,还原回之前的样子就可以了

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

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.

I left some comment.please check it.

script/client/spring/application.yml Show resolved Hide resolved
@@ -44,12 +43,14 @@
public class NacosRegistryServiceImpl implements RegistryService<EventListener> {
private static final String DEFAULT_NAMESPACE = "";
private static final String DEFAULT_CLUSTER = "default";
private static final String DEFAULT_GROUP = "SEATA_GROUP";
Copy link
Member

Choose a reason for hiding this comment

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

DEFAULT_GROUP

Copy link
Member

Choose a reason for hiding this comment

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

high and low version compatibility needs to be considered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

set default value is DEFAULT_GROUP?

Copy link
Contributor

Choose a reason for hiding this comment

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

set default value is DEFAULT_GROUP?

yes,backward compatible

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.

And please fix the conflict.

@slievrly slievrly added the status: waiting-for-feedback Waiting for feedback label Jun 25, 2020
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.

@slievrly slievrly removed the status: waiting-for-feedback Waiting for feedback label Jun 29, 2020
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM

@slievrly slievrly merged commit b2738d0 into apache:develop Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
first-time contributor first-time contributor module/discovery discovery module module/seata-spring-boot-starter seata-spring-boot-starter module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants