Skip to content

Conversation

@KamToHung
Copy link
Contributor

@KamToHung KamToHung commented Sep 20, 2025

#2741

optimization points

1. optimize server.go

the content of the global variable config/service.go was migrated to the server/server.go struct for internal use.

2. optimize server.go params

optimize the Server struct member variable svcOptsMap in server/server.go: change key: *ServiceOptions, value: *common.ServiceInfo to key: service name, value: *ServiceOptions, and add an additional map member variable to store key: interfaceName, value: *ServiceOptions.

old:
image

new:
image

3. dealing with the unreasonable issue of exportServices

In the old code, compatibility issues with Java-Dubbo regarding the case sensitivity of the first letter were handled in the exportServices method within server/server.go. This approach was not very reasonable and has now been migrated to Register for compatibility.

old:
image

new:
image

4. optimize Register and RegisterService method

old:
image

new:
image

5. optimize export

old:
image

new:
image

6. add protocols config compatibility

7. optimize grpc config compatibility

8. optimize Getty config compatibility(by xuetao)

@KamToHung KamToHung changed the base branch from main to develop September 20, 2025 08:10
@codecov-commenter
Copy link

codecov-commenter commented Sep 20, 2025

Codecov Report

❌ Patch coverage is 19.91342% with 185 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.25%. Comparing base (60d1c2a) to head (18a06b2).
⚠️ Report is 646 commits behind head on develop.

Files with missing lines Patch % Lines
server/server.go 8.00% 69 Missing ⚠️
protocol/grpc/client.go 34.37% 20 Missing and 1 partial ⚠️
compat.go 0.00% 20 Missing ⚠️
remoting/getty/getty_client.go 13.63% 17 Missing and 2 partials ⚠️
remoting/getty/getty_server.go 13.63% 17 Missing and 2 partials ⚠️
protocol/grpc/server.go 57.14% 7 Missing and 2 partials ⚠️
common/extension/registry_directory.go 0.00% 7 Missing ⚠️
client/options.go 25.00% 6 Missing ⚠️
remoting/getty/config.go 33.33% 3 Missing and 3 partials ⚠️
server/action.go 0.00% 4 Missing ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3034      +/-   ##
===========================================
- Coverage    46.76%   40.25%   -6.51%     
===========================================
  Files          295      457     +162     
  Lines        17172    32552   +15380     
===========================================
+ Hits          8031    13105    +5074     
- Misses        8287    18184    +9897     
- Partials       854     1263     +409     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AlexStocks AlexStocks requested a review from Copilot September 20, 2025 09:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR optimizes the config.GetConsumerServiceByInterfaceName method by introducing a new mechanism to retrieve consumer services from URL attributes instead of relying solely on the legacy interface name lookup. The changes provide a fallback mechanism where services can be obtained directly from the URL's RpcServiceKey attribute when available.

  • Adds fallback logic to check for RpcServiceKey attribute before using interface name lookup
  • Updates dependency management by moving github.com/spf13/viper from indirect to direct dependency
  • Refactors test cases to use a mock service instead of the original HelloService

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
protocol/triple/dubbo3_invoker.go Adds fallback to retrieve consumer service from URL attribute
protocol/grpc/client.go Adds same fallback mechanism for GRPC client initialization
go.mod Moves viper dependency from indirect to direct
config/service_test.go Updates tests to use MockHelloService instead of HelloService

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@KamToHung
Copy link
Contributor Author

WIP

@KamToHung KamToHung changed the title optimize config.GetConsumerServiceByInterfaceName optimize config Sep 24, 2025
@KamToHung KamToHung closed this Sep 24, 2025
@KamToHung KamToHung reopened this Sep 24, 2025
@KamToHung KamToHung marked this pull request as draft September 25, 2025 17:21
@KamToHung KamToHung marked this pull request as ready for review October 19, 2025 14:24
@KamToHung
Copy link
Contributor Author

PTAL

Comment on lines 48 to 52
// TODO: After the config is removed, remove the test
func InitDubboServer() {
serviceConfig := config.NewServiceConfigBuilder().
SetInterface("org.apache.dubbo.DubboGreeterImpl").
SetProtocolIDs("tripleKey").Build()
Copy link
Contributor

Choose a reason for hiding this comment

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

pls add corresponding test in new api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pls add corresponding test in new api

dubbo3只有config api方式,现在用new triple代替了。加了TODO后续移除

@nanjiek
Copy link
Contributor

nanjiek commented Oct 20, 2025

resolve the conflicts

@Alanxtl
Copy link
Contributor

Alanxtl commented Oct 26, 2025

现在好像是由于冲突问题ci没运行,先解决一下冲突问题,看一下ci结果

# Conflicts:
#	server/server.go
@KamToHung
Copy link
Contributor Author

现在好像是由于冲突问题ci没运行,先解决一下冲突问题,看一下ci结果

已处理

@sonarqubecloud
Copy link

Copy link
Contributor

@nanjiek nanjiek left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants