Skip to content

refactor: Eliminate global state in remoting config and improve architecture clarity#1031

Open
rookiewwj wants to merge 18 commits intoapache:masterfrom
rookiewwj:refactor-config
Open

refactor: Eliminate global state in remoting config and improve architecture clarity#1031
rookiewwj wants to merge 18 commits intoapache:masterfrom
rookiewwj:refactor-config

Conversation

@rookiewwj
Copy link
Copy Markdown

Summary

This PR refactors the configuration architecture in the remoting layer by removing the problematic SeataConfig struct and eliminating global state, resulting in cleaner separation of concerns and improved testability.

Motivation

The previous SeataConfig design had several issues:

  1. Mixed responsibilities: Combined configurations from different layers (remoting, discovery, and getty) into a single struct
  2. Redundant fields: ServiceVgroupMapping and ServiceGrouplist were copied from discovery config but never actually used in remoting
  3. Global state: Configuration was stored in a global variable (seataConfig), making the code harder to test and maintain
  4. Unclear data flow: Dependencies between modules were obscured by global variable access

Changes

Removed

  • SeataConfig struct from config.go
  • Global variable seataConfig and associated InitConfig() / GetSeataConfig() functions
  • Unnecessary configuration copying in client.go
  • Tests for removed functionality

Added

  • New ClientIdentity struct in identity.go to represent client identity information (ApplicationID and TxServiceGroup)

Modified

  • InitGetty() signature: now accepts *ClientIdentity instead of *SeataConfig
  • initSessionManager(): updated to receive and store *ClientIdentity as an instance field
  • SessionManager:
    • Added clientIdentity field to eliminate global config access
    • Now uses g.gettyConf.LoadBalanceType directly instead of accessing global state
    • Uses g.clientIdentity.TxServiceGroup for service lookup
  • listener.go: accesses sessionManager.clientIdentity instead of global GetSeataConfig()

Benefits

  • Better separation of concerns: Each layer manages its own configuration
  • No global state: All dependencies are passed explicitly, improving testability
  • Cleaner data flow: Configuration flows directly from client initialization to the components that need it
  • Reduced coupling: Modules no longer depend on a centralized global configuration
  • Code size reduction: Net reduction of 62 lines of code

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 15, 2025

Codecov Report

❌ Patch coverage is 0% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.66%. Comparing base (55e99b4) to head (5e02003).

Files with missing lines Patch % Lines
pkg/remoting/getty/session_manager.go 0.00% 4 Missing ⚠️
pkg/remoting/getty/listener.go 0.00% 3 Missing ⚠️
pkg/client/client.go 0.00% 2 Missing ⚠️
pkg/remoting/getty/getty_init.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1031   +/-   ##
=======================================
  Coverage   57.66%   57.66%           
=======================================
  Files         267      267           
  Lines       17665    17658    -7     
=======================================
- Hits        10187    10183    -4     
+ Misses       6650     6647    -3     
  Partials      828      828           

☔ 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.

@rookiewwj rookiewwj dismissed a stale review via ea21110 December 19, 2025 08:15
Comment thread pkg/client/client.go

// initRemoting init rpc client
func initRemoting(cfg *Config) {
seataConfig := remoteConfig.SeataConfig{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should keep a naming style similar to remoteConfig.SeataConfig, rather than using getty.ClientIdentity.
Getty is not equivalent to remote — getty is just one implementation of remote.
It can be understood as an abstraction, but the name itself could be changed to something more elegant.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

ok, i will change the name style similar to remoteConfig.SeataConfig

}
}

func TestSeataConfig_InitAndGet(t *testing.T) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You removed the old unit test code, but did not add additional tests to cover the newly added code. As a result, the current test coverage has decreased and more unit tests need to be added.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

fine, i will add more test to cover the newly code

@thunguo
Copy link
Copy Markdown
Contributor

thunguo commented Feb 28, 2026

Plz resolve the conflicts and re-run the integrate-test CI.

lxfeng1997 and others added 3 commits February 28, 2026 19:16
* fix: modified version

* fix: modified version

* feat: remove coverage

* fix: change integrate_test.sh

* chore: 空提交,重新推送分支触发CI/CD

---------

Co-authored-by: FengZhang <zfcode@qq.com>
@github-actions github-actions Bot added documentation Improvements or additions to documentation rm milestone tm integration protocol and removed documentation Improvements or additions to documentation labels Feb 28, 2026
Copy link
Copy Markdown
Contributor

@thunguo thunguo 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants