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.

Sign up for GitHub

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

Jump to bottom

refactor: remove providers and add prefix when use polaris #2274

Merged
merged 5 commits into from
May 11, 2023
Merged

refactor: remove providers and add prefix when use polaris #2274

merged 5 commits into from
May 11, 2023

Conversation

chuntaojun
Copy link
Contributor

@chuntaojun chuntaojun commented Mar 28, 2023 •
edited
Loading

What this PR does:

Which issue(s) this PR fixes:
Fixes apache/dubbo-spi-extensions#204

You should pay attention to items below to ensure your pr passes our ci test
We do not merge pr with ci tests failed

  • All ut passed (run 'go test ./...' in project root)
  • After go-fmt ed , run 'go fmt project' using goland.
  • Golangci-lint passed, run 'sudo golangci-lint run' in project root.
  • Your new-created file needs to have apache license at the top, like other existed file does.
  • All integration test passed. You can run integration test locally (with docker env). Clone our dubbo-go-samples project and replace the go.mod to your dubbo-go, and run 'sudo sh start_integration_test.sh' at root of samples project root. (M1 Slice is not Support)

@chuntaojun chuntaojun changed the title Refactor/register service refactor:remove providers: prefix when use polaris Mar 28, 2023
@chickenlj
Copy link
Contributor

This looks like a bugfix. Do you think this needs to be included in the next 3.0.x release, or it's ok to be just included in 3.1.

@chickenlj chickenlj self-requested a review March 28, 2023 11:29
chickenlj
chickenlj approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@chickenlj chickenlj left a comment

Choose a reason for hiding this comment

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

LGTM.

justxuewei
justxuewei reviewed Mar 28, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Thanks @chuntaojun, but here is a comment.

@AlexStocks AlexStocks changed the title refactor:remove providers: prefix when use polaris refactor: remove providers and add prefix when use polaris Mar 28, 2023
@AlexStocks
Copy link
Contributor

@chuntaojun pls fix the ci failure.

@AlexStocks
Copy link
Contributor

@chuntaojun pls fix the ci failure.

@justxuewei
Copy link
Member

Please pull the latest commits after #2280 is merged to address the CI issues.

@chuntaojun chuntaojun force-pushed the refactor/register_service branch from 03258da to 5a76a5e Compare March 29, 2023 16:28
@github-advanced-security
Copy link

You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab.

@chuntaojun
Copy link
Contributor Author

included in the next 3.0.x release

hope included in the next 3.0.x release，tks

@justxuewei
Copy link
Member

justxuewei commented Apr 3, 2023 •
edited
Loading

hope included in the next 3.0.x release，tks

You should backport this to 3.0 branch. That is, you should open a new pull request whose target branch is 3.0.

Btw, the CI of your PR is not up-to-date. That is why your CI failed. Please pull the latest commit from the main branch and rebase onto it.

For example:

@justxuewei
Copy link
Member

Please rebase your PR onto the latest main branch to address the CI failure.

@chuntaojun chuntaojun force-pushed the refactor/register_service branch from 044d1ad to c8f520a Compare April 7, 2023 12:47
@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@codecov-commenter
Copy link

Codecov Report

Merging #2274 (75f350d) into main (3fcc135) will decrease coverage by 0.27%.
The diff coverage is n/a.

❗ Current head 75f350d differs from pull request most recent head c8f520a. Consider uploading reports for the commit c8f520a to get more accurate results

@@            Coverage Diff             @@
##             main    #2274      +/-   ##
==========================================
- Coverage   44.23%   43.96%   -0.27%     
==========================================
  Files         289      284       -5     
  Lines       17916    17418     -498     
==========================================
- Hits         7925     7658     -267     
+ Misses       9149     8949     -200     
+ Partials      842      811      -31
Impacted Files Coverage Δ
protocol/dubbo3/dubbo3_invoker.go 58.33% <ø> (-1.44%) ⬇️
protocol/dubbo3/dubbo3_protocol.go 56.02% <ø> (-5.09%) ⬇️
protocol/grpc/client.go 60.25% <ø> (-3.75%) ⬇️
protocol/grpc/server.go 63.44% <ø> (-3.23%) ⬇️

... and 10 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

justxuewei
justxuewei approved these changes Apr 9, 2023
View reviewed changes
Copy link
Member

@justxuewei justxuewei left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks! I left one comment here.

registry/polaris/registry.go
request.Namespace = pr.namespace
if err := pr.provider.Deregister(request); err != nil {
return perrors.WithMessagef(err, "register(conf:%+v)", conf)
return perrors.WithMessagef(err, "fail to deregister(conf:%+v)", url)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return perrors.WithMessagef(err, "fail to deregister(conf:%+v)", url)
return perrors.WithMessagef(err, "failed to deregister service from the Polaris registry, url = %v", url)

@justxuewei
Copy link
Member

justxuewei commented Apr 9, 2023 •
edited
Loading

Btw, please link an issue, see here CONTRIBUTING.md.

@AlexStocks
Copy link
Contributor

Btw, please link an issue, see here CONTRIBUTING.md.

agree with you. @chuntaojun

@chuntaojun
Copy link
Contributor Author

already link issue

@AlexStocks AlexStocks merged commit 8f8f514 into apache:main May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexStocks AlexStocks AlexStocks approved these changes

@justxuewei justxuewei justxuewei approved these changes

@chickenlj chickenlj chickenlj approved these changes

@jasondeng1997 jasondeng1997 jasondeng1997 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

使用dubbo-registry-polaris接入polaris与dubbogo互相不能发现服务
6 participants
@chuntaojun @chickenlj @AlexStocks @justxuewei @codecov-commenter @jasondeng1997