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

feat:remove old zookeeper #14028

Merged
merged 13 commits into from
May 15, 2024
Merged

Conversation

Stellar1999
Copy link
Contributor

What is the purpose of the change

refer:#13960

Brief changelog

Verifying this change

Checklist

  • Make sure there is a GitHub_issue field for the change (usually before you start working on it). Trivial changes like typos do not require a GitHub issue. Your pull request should address just this issue, without pulling in other changes - one PR resolves one issue.
  • Each commit in the pull request should have a meaningful subject line and body.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Check if is necessary to patch to Dubbo 3 if you are work on Dubbo 2.7
  • Write necessary unit-test to verify your logic correction, more mock a little better when cross module dependency exist. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Add some description to dubbo-website project if you are requesting to add a feature.
  • GitHub Actions works fine on your own branch.
  • If this contribution is large, please follow the Software Donation Guide.

@Stellar1999
Copy link
Contributor Author

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);
Copy link
Member

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.

Copy link
Contributor Author

@Stellar1999 Stellar1999 Apr 8, 2024

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.

Copy link
Member

@CrazyHZM CrazyHZM Apr 9, 2024

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.

Copy link
Contributor Author

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-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.37%. Comparing base (a12975a) to head (cb220be).
Report is 1 commits behind head on 3.3.

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.
📢 Have feedback on the report? Share it here.

@Stellar1999 Stellar1999 force-pushed the feat/remove_old_zookeeper branch 5 times, most recently from 5ceb6e4 to 32d7c54 Compare April 8, 2024 07:22
@AlbumenJ AlbumenJ requested a review from CrazyHZM April 8, 2024 08:32
@Stellar1999 Stellar1999 force-pushed the feat/remove_old_zookeeper branch 4 times, most recently from 756897a to cc89d88 Compare April 9, 2024 09:30
@Stellar1999 Stellar1999 force-pushed the feat/remove_old_zookeeper branch from 111a0b2 to 114dad8 Compare April 9, 2024 12:18
@CrazyHZM
Copy link
Member

@Stellar1999
We should keep the dubbo-remoting-zookeeper-curator5 module and move the dubbo-remoting-zookeeper-api module into it.

@Stellar1999 Stellar1999 force-pushed the feat/remove_old_zookeeper branch from 4375e66 to 6b06312 Compare April 11, 2024 12:51
@Stellar1999
Copy link
Contributor Author

@Stellar1999 We should keep the dubbo-remoting-zookeeper-curator5 module and move the dubbo-remoting-zookeeper-api module into it.

Please take another look. Thanks for your guidance. @CrazyHZM

@AlbumenJ
Copy link
Member

AlbumenJ commented May 6, 2024

image

@CrazyHZM
Copy link
Member

CrazyHZM commented May 8, 2024

It all looks good at the moment, but recently, CI has not been very stable. Wait until CI is fully passed before merging.
@AlbumenJ

@AlbumenJ
Copy link
Member

Please resolve conflicts

# Conflicts:
#	dubbo-dependencies/dubbo-dependencies-zookeeper/pom.xml
Copy link

@AlbumenJ AlbumenJ merged commit e826b0c into apache:3.3 May 15, 2024
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants