Skip to content

feature: expose TC version to client side via ServerVersionHolder - #8183

Open
WangzJi wants to merge 6 commits into
apache:2.xfrom
WangzJi:feature/client-get-server-version
Open

feature: expose TC version to client side via ServerVersionHolder#8183
WangzJi wants to merge 6 commits into
apache:2.xfrom
WangzJi:feature/client-get-server-version

Conversation

@WangzJi

@WangzJi WangzJi commented Jul 25, 2026

Copy link
Copy Markdown
Member

Ⅰ. Describe what this PR did

When a RM/TM registers to a TC, the TC always echoes its own version back in the register response (AbstractIdentifyResponse#version, transferred by both the v0/v1 and the v2 codec as well as by the protobuf convertor). The client, however, only wrote that version into a log line, so upper layers had no way to know which features the target server supports. This is the client side counterpart of RpcContext#getVersion(), which the server has been using for compatibility decisions such as ServerOnRequestProcessor and AbstractNettyRemotingServer.

This PR adds that missing context:

  • Add ServerVersionHolder (org.apache.seata.core.protocol), keyed by the server address used to connect, so that any module can ask whether a given TC is above or equal to a version. An unknown version is deliberately treated as "not satisfied", so a feature relying on the check stays disabled until the version is really known.
  • Record the server version on both RM and TM registration. RmNettyRemotingClient fed a private map introduced by feature: add UnregisterRM protocol to notify server on client destroy #8020, while TmNettyRemotingClient did not record the version at all. Both now write to the holder.
  • Remove the package private serverVersionMap in NettyClientChannelManager. It was only reachable from within org.apache.seata.core.rpc.netty and only fed by the RM path, so it could not serve feature detection outside the rpc layer. Its only consumer, the UnregisterRMRequest version check, now uses the holder.
  • Pass the peer version to registerChannel. The client used to store its own version in Version#VERSION_MAP keyed by the server address, while the server stores the peer version keyed by the client address. Version#getChannelVersion now means "the version of the peer" on both sides.

Entries in the holder are written on every successful registration and are never removed on disconnection. This is intentional: TM and RM reach the same servers through two independent channel managers, so removing an entry when one of them disconnects would break feature detection for the other one. A version is an intrinsic attribute of the server at a given address, and a stale entry can never be read for a live request, because a request always goes through a channel whose registration has just refreshed the entry.

Ⅱ. Does this pull request fix one issue?

No.

Ⅲ. Why don't you add test cases (unit test/integration test)?

Test cases are added:

  • ServerVersionHolderTest: put/get, overwrite on re-registration, blank input ignored, unknown server treated as not satisfied, clear.
  • TmNettyClientTest#onRegisterMsgSuccessRecordsServerVersionTest: new, covers the previously missing TM path.
  • RmNettyClientTest: asserts that the peer version is the one handed to registerChannel and that it lands in the holder; the UnregisterRMRequest version cases now drive the holder instead of stubbing the removed manager method.
  • NettyClientChannelManagerTest: the pure version cases moved to ServerVersionHolderTest, the destroy/cleanup cases keep asserting the channel, pool key and lock cleanup.

Ⅳ. Describe how to verify it

./mvnw -pl core -am test -Dtest='ServerVersionHolderTest,VersionTest,NettyClientChannelManagerTest,RmNettyClientTest,TmNettyClientTest'

For a behavioural check, start a TC and a client, and observe that after registration ServerVersionHolder.getServerVersion(serverAddress) returns the TC version for both TM and RM connections, and that the UnregisterRMRequest on client destroy is still sent only to servers of 2.6.0 and above.

Ⅴ. Special notes for reviews

The change to registerChannel is the only behavioural change outside the new holder, and it is safe: the sole client side reader of Version#VERSION_MAP is MsgVersionHelper#versionNotSupport, whose SKIP_MSG_CODE_V0 contains only TYPE_RM_DELETE_UNDOLOG, a message sent by the TC to the RM and never in the opposite direction. The check is therefore inert on the client side today, and correcting the stored version aligns the two sides instead of changing runtime behaviour.

ServerVersionHolder is kept separate from Version on purpose. Version#VERSION_MAP is keyed by channel.remoteAddress(), whereas the holder is keyed by the server address obtained from the registry; the two are not always equal (NAT, proxy, container port mapping), so merging them into one class would invite mixing the keys up.

Version#VERSION_2_6_0 is promoted from private to public so that the comparison target can be expressed as a constant at the call site.

WangzJi added 2 commits July 25, 2026 15:02
TC always echoes its version back in the register response, but the client
only logged it, so upper layers had no way to know which features the target
server supports.

- add ServerVersionHolder, keyed by server address, as the client side
  counterpart of RpcContext#getVersion() on the server side
- record the server version on both RM and TM registration, the TM side was
  missing entirely
- drop the package private serverVersionMap in NettyClientChannelManager,
  which was only reachable from the netty package and only fed by RM
- pass the peer version to registerChannel, so that Version#VERSION_MAP means
  the version of the peer on both the client and the server side
@codecov

codecov Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.08%. Comparing base (5ba6a62) to head (b7bde29).
⚠️ Report is 3 commits behind head on 2.x.

Files with missing lines Patch % Lines
...pache/seata/core/protocol/ServerVersionHolder.java 91.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                2.x    #8183      +/-   ##
============================================
+ Coverage     73.03%   73.08%   +0.04%     
  Complexity     1141     1141              
============================================
  Files          1152     1153       +1     
  Lines         42288    42308      +20     
  Branches       5046     5047       +1     
============================================
+ Hits          30887    30922      +35     
+ Misses         8924     8910      -14     
+ Partials       2477     2476       -1     
Files with missing lines Coverage Δ
...n/java/org/apache/seata/core/protocol/Version.java 86.36% <ø> (ø)
...eata/core/rpc/netty/NettyClientChannelManager.java 83.72% <ø> (-0.73%) ⬇️
...he/seata/core/rpc/netty/RmNettyRemotingClient.java 72.41% <100.00%> (+0.63%) ⬆️
...he/seata/core/rpc/netty/TmNettyRemotingClient.java 83.20% <100.00%> (+5.86%) ⬆️
...pache/seata/core/protocol/ServerVersionHolder.java 91.66% <91.66%> (ø)

... and 5 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI left a comment

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.

Pull request overview

This PR adds a client-side mechanism to persist and query the connected TC’s version (keyed by the configured server address), and updates RM/TM registration paths to record the peer (server) version consistently for feature detection and compatibility checks.

Changes:

  • Introduce ServerVersionHolder to store TC versions by server address and provide a version-gate helper (isServerAboveOrEqualVersion).
  • Record server version on both RM and TM registration success; remove the old RM-only serverVersionMap from NettyClientChannelManager and switch UnregisterRMRequest gating to the new holder.
  • Align registerChannel(..., version) to store the peer version and update tests + changelog entries accordingly.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
core/src/main/java/org/apache/seata/core/protocol/ServerVersionHolder.java New shared holder for server(TC) versions keyed by configured server address, with helper for version gating.
core/src/main/java/org/apache/seata/core/protocol/Version.java Exposes VERSION_2_6_0 as a public constant for call-site comparisons.
core/src/main/java/org/apache/seata/core/rpc/netty/RmNettyRemotingClient.java Stores server version into ServerVersionHolder; uses holder to gate UnregisterRMRequest; removes old per-manager clearing behavior.
core/src/main/java/org/apache/seata/core/rpc/netty/TmNettyRemotingClient.java Records server version on TM registration success and passes peer version into registerChannel.
core/src/main/java/org/apache/seata/core/rpc/netty/NettyClientChannelManager.java Removes internal serverVersionMap; documents that registerChannel stores the peer version.
core/src/test/java/org/apache/seata/core/protocol/ServerVersionHolderTest.java New unit tests for holder put/get/overwrite/blank handling/version gating/clear behavior.
core/src/test/java/org/apache/seata/core/rpc/netty/TmNettyClientTest.java Adds a test ensuring TM registration records server version and passes peer version to registerChannel.
core/src/test/java/org/apache/seata/core/rpc/netty/RmNettyClientTest.java Updates assertions to verify peer version is registered and stored in the holder; drives unregister gating through the holder.
core/src/test/java/org/apache/seata/core/rpc/netty/NettyClientChannelManagerTest.java Removes tests tied to the deleted serverVersionMap, keeps channel/pool/lock cleanup tests.
changes/en-us/2.x.md Adds changelog entry for exposing TC version via ServerVersionHolder.
changes/zh-cn/2.x.md Adds corresponding Chinese changelog entry.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +87 to +92
public static boolean isServerAboveOrEqualVersion(String serverAddress, String targetVersion) {
String serverVersion = getServerVersion(serverAddress);
if (StringUtils.isBlank(serverVersion)) {
return false;
}
return Version.isAboveOrEqualVersion(serverVersion, targetVersion);

@slievrly slievrly 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.

Nice consolidation — the client-side RpcContext#getVersion() counterpart is genuinely missing today, and fixing the registerChannel(..., version) argument to be the peer's version (not the client's own) closes a real inconsistency with the server side. The ServerVersionHolder API is clean and the test coverage is solid.

One thing I'd like to push back on though: why remove the cleanup mechanism entirely?

The old per-manager serverVersionMap was removed on destroyChannel / cleanupDisconnectedChannelMetadata / destroy(). The new ServerVersionHolder is a JVM-wide static that is only ever written to — nothing in production code calls clear(), and the disconnect / destroy paths no longer touch it. The PR body explains the rationale (TM and RM are two independent channel managers, so removing on one disconnect would break feature detection for the other), and that reasoning is internally consistent for a shared holder. But it trades one problem for a different one:

1. Unbounded growth over the JVM lifetime

Every server address the client has ever registered to stays in the map forever. In a stable cluster this is a few entries. In deployments where TC addresses rotate — blue/green rollouts, autoscaling, Kubernetes pod IPs, registry churn — every historical address accumulates. Each entry is small, but there is no upper bound and no eviction. That's a slow leak the old design didn't have.

2. Static global state with no destroy hook

The old serverVersionMap was owned by the channel manager and cleaned up on destroy(). ServerVersionHolder's state now outlives every client instance. In embedded scenarios where the Seata client is torn down and re-initialized in the same JVM (uncommon but real — hot reload, test harness, multi-tenant containers), the second instance inherits the first instance's map. If the TC at a given address was replaced between the two instantiations, the stale version wins until the next successful registration overwrites it. ServerVersionHolder.clear() exists but is only called from @BeforeEach in the test — nothing calls it in production.

3. Suggested alternatives

Any of these keeps the "don't break the other manager on single-manager disconnect" invariant while restoring a cleanup path:

  • Reference-counting on the shared holder. Track which managers have registered each address (e.g. a Set<String> managerIds per address); remove the version entry only when the count drops to zero. Cleanup then happens automatically when the last manager for that address disconnects.
  • Cleanup on client-level destroy, not per-channel disconnect. Keep per-channel disconnects as no-ops (as this PR does), but have the RM and TM clients coordinate a "when both are destroyed, clear the holder" hook. Simpler than ref-counting, still bounded.
  • Bounded cache instead of ConcurrentHashMap. A small size cap (e.g. Caffeine with maximumSize=1024) makes the memory footprint statically bounded without requiring lifecycle coordination.

If the deliberate choice is "we accept the leak because in practice the address set is small", it'd be worth calling that out explicitly in the Javadoc so a future reader doesn't try to add cleanup and re-introduce the TM/RM interference bug.

Not blocking — just want to make sure the removal of the existing cleanup is a considered decision rather than a side-effect of moving to a shared holder.

@WangzJi

WangzJi commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

@slievrly Thanks for the thoughtful feedback. I agree that the shared holder needs a production cleanup path.

I've added client-lifecycle coordination: RM and TM attach on initialization and detach on destroy; versions are cleared only after the last active client is destroyed. This preserves the shared state across a single manager/channel disconnect while preventing stale entries from surviving a full client teardown. Tests cover both cases.

@WangzJi WangzJi added this to the 2.8.0 milestone Jul 29, 2026
@WangzJi WangzJi added type: feature Category issues or prs related to feature request. module/core core module labels Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module/core core module type: feature Category issues or prs related to feature request.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants