-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Registry polaris #1147
Registry polaris #1147
Conversation
Hi @ZLBer, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
ZLB niu b! |
1 similar comment
ZLB niu b! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
感谢 PR,我看目前是有一个示例,这个能否补充一个单测呢?
example/src/test/java/com/alipay/sofa/rpc/polaris/PolarisBoltClientMain.java
Outdated
Show resolved
Hide resolved
example/src/test/java/com/alipay/sofa/rpc/polaris/PolarisBoltServerMain.java
Outdated
Show resolved
Hide resolved
单测在 registry/registry-polaris/src/test/java/com/alipay/sofa/rpc/registry/polaris/PolarisRegistryTest.java |
@ZLBer Welcome, build fail in PMD check. The detail info |
@EvenLjj done~ |
@ZLBer Please run |
@EvenLjj sorry,formatted it |
Codecov Report
@@ Coverage Diff @@
## master #1147 +/- ##
============================================
+ Coverage 71.08% 71.12% +0.03%
- Complexity 825 826 +1
============================================
Files 407 407
Lines 17199 17199
Branches 2681 2681
============================================
+ Hits 12226 12232 +6
+ Misses 3597 3589 -8
- Partials 1376 1378 +2
Continue to review full report at Codecov.
|
hello。这里sofa-rpc社区的同学能辛苦在帮忙review看看吗 |
@ZLBer @chuntaojun Sorry for the delay, we will review as soon as possible. If there is no problem with the review, this will release in a rpc feature release version. |
...try/registry-polaris/src/main/java/com/alipay/sofa/rpc/registry/polaris/PolarisRegistry.java
Show resolved
Hide resolved
...try/registry-polaris/src/main/java/com/alipay/sofa/rpc/registry/polaris/PolarisRegistry.java
Outdated
Show resolved
Hide resolved
...try/registry-polaris/src/main/java/com/alipay/sofa/rpc/registry/polaris/PolarisRegistry.java
Outdated
Show resolved
Hide resolved
...try/registry-polaris/src/main/java/com/alipay/sofa/rpc/registry/polaris/PolarisRegistry.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
希望可以快点合并到sofa-rpc仓库主分支中去~ |
# Conflicts: # example/pom.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some bug need to be fixed.
getAllInstancesRequest.setNamespace(nameSpace); | ||
getAllInstancesRequest.setService(serviceName); | ||
this.currentData = consumerAPI.getAllInstance(getAllInstancesRequest); | ||
this.watchExecutor = Executors.newSingleThreadScheduledExecutor(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's too expensive that each consumer holds a thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OrezzerO so, should exceute them in one threadPool ?
Motivation:
add a new registry component polaris , the project website : https://github.com/polarismesh/polaris , https://polarismesh.cn/#/
related issue: polarismesh/polaris#163
Modification:
add a new registry component polaris, the code logic is similar to consul registry.
Result:
Fixes #1141 .