Skip to content

refactor: converge protocol and triple configuration models - #3613

Open
EmptyCity-111 wants to merge 3 commits into
apache:developfrom
EmptyCity-111:refactor/issue-3598-converge-protocol-config
Open

refactor: converge protocol and triple configuration models#3613
EmptyCity-111 wants to merge 3 commits into
apache:developfrom
EmptyCity-111:refactor/issue-3598-converge-protocol-config

Conversation

@EmptyCity-111

@EmptyCity-111 EmptyCity-111 commented Aug 7, 2026

Copy link
Copy Markdown

Description

Related to #3598, task 5.

Triple server message-size limits currently have two configuration sources:
deprecated top-level fields in ProtocolConfig and the corresponding fields
under TripleConfig. server/action.go historically forwarded the
protocol-level values as URL parameters, while the Triple server later applied
the nested configuration.

This change:

  • makes TripleConfig the runtime source of truth for Triple message-size
    settings;
  • moves the 4mib receive-size default to TripleConfig;
  • preserves the deprecated ProtocolConfig fields throughout v3 for source
    and YAML compatibility;
  • migrates non-empty legacy values into empty nested fields, while explicit
    nested values take precedence;
  • continues populating the legacy URL parameters for downstream protocol
    compatibility;
  • updates the JSON Schema and regression tests.

Compatibility

The two deprecated exported fields remain available in v3 and are still cloned
and accepted from legacy YAML. Existing code using
ProtocolConfig.MaxServerSendMsgSize or
ProtocolConfig.MaxServerRecvMsgSize therefore continues to compile.

When both legacy and nested values are configured, the non-empty
TripleConfig value wins. The deprecated fields remain scheduled for removal
in v4.

Validation

Validated after rebasing onto the latest develop branch:

go test . ./global ./server ./protocol/triple -count=1
go test ./... -count=1
jq empty tools/dubbo-go-schema/dubbo-go.json
git diff --check upstream/develop...HEAD

All checks pass.

Checklist

  • The target branch is develop.
  • The branch is rebased onto the latest develop.
  • Code has passed focused and full-repository tests.
  • Tests cover defaults, cloning, legacy YAML migration, value precedence,
    exported URL parameters, and the real Triple server message-size path.

@Alanxtl

Alanxtl commented Aug 7, 2026

Copy link
Copy Markdown
Member

pls fix ci fail

@Alanxtl Alanxtl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@EmptyCity-111
EmptyCity-111 marked this pull request as ready for review August 8, 2026 09:00
@codecov-commenter

codecov-commenter commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.00%. Comparing base (60d1c2a) to head (672fd79).
⚠️ Report is 902 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3613      +/-   ##
===========================================
+ Coverage    46.76%   54.00%   +7.23%     
===========================================
  Files          295      474     +179     
  Lines        17172    36410   +19238     
===========================================
+ Hits          8031    19662   +11631     
- Misses        8287    15166    +6879     
- Partials       854     1582     +728     

☔ View full report in Codecov by Harness.
📢 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.

Comment thread global/protocol_config.go
Comment thread server/action.go
@EmptyCity-111

Copy link
Copy Markdown
Author

Hi @Alanxtl, the main CI (format, unit tests, lint, and Codecov) is green. The Integration Test failed again only at the observability assertion Prometheus scrape targets are up; RPC calls, the expected error path, Prometheus metrics, Jaeger tracing, and Grafana provisioning all passed.

This is the same environment-dependent failure as the previous run and is unrelated to the protocol configuration diff. I tried to rerun the failed job, but GitHub requires repository admin permission. Could you please rerun the failed Integration Test job when convenient?

Job: https://github.com/apache/dubbo-go/actions/runs/31297924714/job/93206026829

@AlexStocks

Copy link
Copy Markdown
Contributor

@EmptyCity-111 the target branch of your PR should be 'develop'.

Keep Triple-specific message size settings under TripleConfig and move the receive-size default with them. Continue populating legacy URL parameters so downstream protocol handling keeps working, and align the schema and tests with the nested model.

Refs: apache#3598
Signed-off-by: EmptyCity-111 <3047874865@qq.com>
Signed-off-by: EmptyCity-111 <3047874865@qq.com>
Keep deprecated ProtocolConfig message size fields available through v3, migrate their values into TripleConfig when nested values are empty, and preserve nested precedence during export. Add YAML, URL propagation, external compilation, and real Triple request-size regression coverage.

Signed-off-by: EmptyCity-111 <3047874865@qq.com>
@EmptyCity-111
EmptyCity-111 force-pushed the refactor/issue-3598-converge-protocol-config branch from 3b4437e to 672fd79 Compare August 10, 2026 11:43
@sonarqubecloud

Copy link
Copy Markdown

@EmptyCity-111
EmptyCity-111 changed the base branch from main to develop August 10, 2026 11:49
Comment thread server/action.go
Comment on lines -204 to -205
// fix https://github.com/apache/dubbo-go/issues/2176
// TODO: remove MaxServerSendMsgSize value and MaxServerRecvMsgSize value when version 4.0.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

keep this todo

Comment thread global/triple_config.go
Http3: DefaultHttp3Config(),
Cors: DefaultCorsConfig(),
OpenAPI: DefaultOpenAPIConfig(),
MaxServerRecvMsgSize: "4mib",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

move this 4mib to common/const

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[OSPP] 完善 Dubbo-Go SDK 文档与 dubbo-go 入门任务

4 participants