-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
feat:remove old zookeeper #14028
feat:remove old zookeeper #14028
Conversation
Should the first CI test fix be merged, then rerun the test case for this PR. In my own repository, it has already passed. |
ZookeeperClient connect(URL url); | ||
|
||
void destroy(); | ||
|
||
static ZookeeperTransporter getExtension(ApplicationModel applicationModel) { | ||
ExtensionLoader<ZookeeperTransporter> extensionLoader = | ||
applicationModel.getExtensionLoader(ZookeeperTransporter.class); | ||
return isHighVersionCurator() ? extensionLoader.getExtension(CURATOR_5) : extensionLoader.getExtension(CURATOR); | ||
return extensionLoader.getExtension(CURATOR_5); |
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.
The existence of ZookeeperTransporter is not very meaningful, so consider removing it.
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.
Please take another look. Do you think my code idea is correct? I removed the SPI and moved the ZookeeperTransporter to the module dubbo-remoting-zookeeper-curator5
in order to create an instance of Curator5ZookeeperTransporter. @CrazyHZM Thanks for your code review.
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.
@Stellar1999 ZookeeperTransporter should be renamed ZookeeperClientManager or something else.
dubbo-remoting-zookeeper-api should also be incorporated into the curator5 module.
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.
Please take another look. Thanks for your guidance. @CrazyHZM
If it's alright, I will fix the sample test case.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 3.3 #14028 +/- ##
==========================================
- Coverage 38.55% 38.37% -0.18%
==========================================
Files 1895 1893 -2
Lines 79272 79016 -256
Branches 11528 11502 -26
==========================================
- Hits 30560 30321 -239
- Misses 44439 44443 +4
+ Partials 4273 4252 -21 ☔ View full report in Codecov by Sentry. |
5ceb6e4
to
32d7c54
Compare
756897a
to
cc89d88
Compare
111a0b2
to
114dad8
Compare
@Stellar1999 |
# Conflicts: # dubbo-dependencies/dubbo-dependencies-zookeeper/pom.xml
4375e66
to
6b06312
Compare
Please take another look. Thanks for your guidance. @CrazyHZM |
It all looks good at the moment, but recently, CI has not been very stable. Wait until CI is fully passed before merging. |
Please resolve conflicts |
# Conflicts: # dubbo-dependencies/dubbo-dependencies-zookeeper/pom.xml
|
What is the purpose of the change
refer:#13960
Brief changelog
Verifying this change
Checklist