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

feat: metadata port reuse #2538

Closed
wants to merge 1 commit into from
Closed

feat: metadata port reuse #2538

wants to merge 1 commit into from

Conversation

baerwang
Copy link
Member

@baerwang baerwang added the enhancement New feature or request label Dec 12, 2023
@FinalT
Copy link
Member

FinalT commented Dec 13, 2023

Please commit to Branch 3.1

@baerwang baerwang changed the base branch from main to release-3.1 December 13, 2023 05:29
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2023 •
edited
Loading

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (d204c4e) 44.91% compared to head (129f2ec) 44.73%.

❗ Current head 129f2ec differs from pull request most recent head 45b2ee7. Consider uploading reports for the commit 45b2ee7 to get more accurate results

Files Patch % Lines
config/service_config.go 0.00% 2 Missing ⚠️
protocol/dubbo/dubbo_protocol.go 50.00% 1 Missing ⚠️
protocol/protocol.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@               Coverage Diff               @@
##           release-3.1    #2538      +/-   ##
===============================================
- Coverage        44.91%   44.73%   -0.18%     
===============================================
  Files              270      270              
  Lines            18354    18355       +1     
===============================================
- Hits              8243     8211      -32     
- Misses            9225     9262      +37     
+ Partials           886      882       -4

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

@baerwang baerwang force-pushed the feat/port-reuse branch 2 times, most recently from 7038aa8 to dd586fb Compare December 13, 2023 06:38
@baerwang baerwang requested review from FoghostCn and FinalT December 13, 2023 06:39
FinalT
FinalT reviewed Dec 13, 2023
View reviewed changes
metadata/service/exporter/configurable/exporter.go
Comment on lines +66 to +72
protocolConfig, ok := config.GetRootConfig().Protocols[constant.DefaultProtocol]
if !ok {
protocolConfig = config.NewProtocolConfigBuilder().
SetName(constant.DefaultProtocol).
SetPort(strconv.Itoa(config.GetApplicationConfig().MetadataServicePort)).
Build()
}
Copy link
Member

Choose a reason for hiding this comment

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

If the user sets the MetadataServicePort, should we use that port？

Copy link
Member Author

Choose a reason for hiding this comment

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

consistent with java

Copy link
Contributor

@FoghostCn FoghostCn Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

since we want reuse dubbo protocol port, I think the priority should be

  1. MetadataServicePort
  2. dubbo protocol port set by user or default

Copy link
Member Author

Choose a reason for hiding this comment

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

use default port,ci fail

Copy link
Contributor

Choose a reason for hiding this comment

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

use default port,ci fail

why

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/apache/dubbo-go/actions/runs/7203720804/job/19624025841

Copy link
Contributor

Choose a reason for hiding this comment

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

please create a new branch to show how you use default port,I want to know why ci failed

Copy link
Member Author

Choose a reason for hiding this comment

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

d08bf0f

@baerwang baerwang requested review from FinalT and FoghostCn December 13, 2023 08:20
@baerwang baerwang force-pushed the feat/port-reuse branch 3 times, most recently from d08bf0f to 3dc7598 Compare December 14, 2023 02:39
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@baerwang
Copy link
Member Author

#2548

@baerwang baerwang closed this Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FoghostCn FoghostCn Awaiting requested review from FoghostCn

@FinalT FinalT Awaiting requested review from FinalT

@zhaoyunxing92 zhaoyunxing92 Awaiting requested review from zhaoyunxing92

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@baerwang @FinalT @codecov-commenter @FoghostCn @zhaoyunxing92