You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "rookiewwj (via GitHub)" <gi...@apache.org> on 2025/12/15 08:31:37 UTC
[PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
rookiewwj opened a new pull request, #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031
### 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "tew-axiom (via GitHub)" <gi...@apache.org>.
tew-axiom commented on code in PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#discussion_r2622398532
##########
pkg/remoting/getty/identity.go:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package getty
+
+// ClientIdentity represents the identity of a seata client,
+// including application ID and transaction service group.
+type ClientIdentity struct {
+ ApplicationID string
+ TxServiceGroup string
+}
Review Comment:
Can we just put `ClientIdentity` directly in `session_manager`? It seems that it's only directly used in `session_manager`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "Code-Fight (via GitHub)" <gi...@apache.org>.
Code-Fight commented on code in PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#discussion_r2636931304
##########
pkg/remoting/config/config_test.go:
##########
@@ -161,64 +159,3 @@ func TestTransportConfig_RegisterFlagsWithPrefix(t *testing.T) {
})
}
}
-
-func TestSeataConfig_InitAndGet(t *testing.T) {
Review Comment:
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.
##########
pkg/client/client.go:
##########
@@ -65,15 +64,12 @@ func initTmClient(cfg *Config) {
// initRemoting init rpc client
func initRemoting(cfg *Config) {
- seataConfig := remoteConfig.SeataConfig{
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "flypiggyyoyoyo (via GitHub)" <gi...@apache.org>.
flypiggyyoyoyo commented on PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#issuecomment-3677790857
Plz reply the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "rookiewwj (via GitHub)" <gi...@apache.org>.
rookiewwj commented on code in PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#discussion_r2622651153
##########
pkg/remoting/getty/identity.go:
##########
@@ -0,0 +1,25 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package getty
+
+// ClientIdentity represents the identity of a seata client,
+// including application ID and transaction service group.
+type ClientIdentity struct {
+ ApplicationID string
+ TxServiceGroup string
+}
Review Comment:
Do you mean dropping ClientIdentity and directly using ApplicationID and TxServiceGroup?
I think keeping ClientIdentity is a bit cleaner, both in terms of code readability and overall code cleanliness, and it also keeps the parameters passed to InitGetty more concise.
If we remove ClientIdentity, the main impact would be on the parameters of InitGetty; from an implementation perspective, it would be easy to change.
Which approach do you think we should use?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "flypiggyyoyoyo (via GitHub)" <gi...@apache.org>.
flypiggyyoyoyo commented on PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#issuecomment-3677790928
Plz reply the comments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "thunguo (via GitHub)" <gi...@apache.org>.
thunguo commented on PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#issuecomment-3976760134
Plz resolve the conflicts and re-run the `integrate-test` CI.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#issuecomment-3654700369
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `0%` with `13 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 61.67%. Comparing base ([`774046b`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/774046b21a81259e56b8d2825276276e94a410fa?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e5e6ba8`](https://app.codecov.io/gh/apache/incubator-seata-go/commit/e5e6ba867b48cec68bd2814e4b9c0aeea22d9357?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [pkg/client/client.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&filepath=pkg%2Fclient%2Fclient.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NsaWVudC9jbGllbnQuZ28=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/remoting/getty/session\_manager.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&filepath=pkg%2Fremoting%2Fgetty%2Fsession_manager.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3JlbW90aW5nL2dldHR5L3Nlc3Npb25fbWFuYWdlci5nbw==) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/remoting/getty/listener.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&filepath=pkg%2Fremoting%2Fgetty%2Flistener.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3JlbW90aW5nL2dldHR5L2xpc3RlbmVyLmdv) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [pkg/remoting/getty/getty\_init.go](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&filepath=pkg%2Fremoting%2Fgetty%2Fgetty_init.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3JlbW90aW5nL2dldHR5L2dldHR5X2luaXQuZ28=) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #1031 +/- ##
=======================================
Coverage 61.67% 61.67%
=======================================
Files 213 213
Lines 13059 13052 -7
=======================================
- Hits 8054 8050 -4
+ Misses 4481 4478 -3
Partials 524 524
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/incubator-seata-go/pull/1031?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "tew-axiom (via GitHub)" <gi...@apache.org>.
tew-axiom commented on PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#issuecomment-3673985368
Plz resolve the conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "rookiewwj (via GitHub)" <gi...@apache.org>.
rookiewwj commented on code in PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#discussion_r2637097219
##########
pkg/client/client.go:
##########
@@ -65,15 +64,12 @@ func initTmClient(cfg *Config) {
// initRemoting init rpc client
func initRemoting(cfg *Config) {
- seataConfig := remoteConfig.SeataConfig{
Review Comment:
ok, i will change the name style similar to remoteConfig.SeataConfig
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "rookiewwj (via GitHub)" <gi...@apache.org>.
rookiewwj commented on code in PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031#discussion_r2637097325
##########
pkg/remoting/config/config_test.go:
##########
@@ -161,64 +159,3 @@ func TestTransportConfig_RegisterFlagsWithPrefix(t *testing.T) {
})
}
}
-
-func TestSeataConfig_InitAndGet(t *testing.T) {
Review Comment:
fine, i will add more test to cover the newly code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: Eliminate global state in remoting config and improve architecture clarity [incubator-seata-go]
Posted by "thunguo (via GitHub)" <gi...@apache.org>.
thunguo merged PR #1031:
URL: https://github.com/apache/incubator-seata-go/pull/1031
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org