feature : Integrate Raft and Protocol Switching Based on Server Version #7586

YongGoose · 2025-08-15T13:15:08Z

I have read the CONTRIBUTING.md guidelines.
I have registered the PR changes.

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

fixes #7544

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Signed-off-by: yongjunhong <[email protected]>

YongGoose · 2025-08-15T13:21:07Z

@funky-eyes

I first select the Node in order to determine whether to use HTTP/2 or HTTP/1, and then extract the address accordingly.

There’s one decision point we need to address.
With HTTP/1, I can process the result after receiving the response, whereas with HTTP/2, the result is handled via a callback.
I’m currently considering how to make the behavior consistent between the two.

If you have any suggestions for a good approach, Don't hesitate to tell me :)

YongGoose · 2025-08-15T13:23:31Z

...ery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java

-    private static String queryHttpAddress(String clusterName, String group) {
+    // 그러면 여기에서 string을 반환하는 것이 아니라 node를 반환해야 된다. (node를 반환해서 node의 버전에 따라 결정)
+    // 또는 2.0이 가능한 node를 먼저 선택하게 하는 거는 어떨까?
+    private static Node selectNodeForHttpAddress(String clusterName, String group) {


This method is designed to select the node.

YongGoose · 2025-08-15T13:24:01Z

...ery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java

+        return null;
+    }
+
+    private static String queryHttpAddress(String clusterName, Node selectedNode) {


This method extracts the HTTP address based on the selected node and the cluster name.

YongGoose · 2025-08-15T13:25:53Z

...ery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java

+        if (selectedNode.isHttp2Supported()) {
+            Http2ClientUtil.doPost("http://" + tcAddress + "/metadata/v1/watch", param, header, CALLBACK);
+            return true; // TODO : Handle the response properly
+        }


So that HTTP/2 is used based on the node’s version.

funky-eyes · 2025-08-18T06:33:39Z

@funky-eyes

I first select the Node in order to determine whether to use HTTP/2 or HTTP/1, and then extract the address accordingly.

There’s one decision point we need to address. With HTTP/1, I can process the result after receiving the response, whereas with HTTP/2, the result is handled via a callback. I’m currently considering how to make the behavior consistent between the two.

If you have any suggestions for a good approach, Don't hesitate to tell me :)

I agree, because during initialization, the server-side API is called to obtain metadata about the members in the Raft group. At this point, it’s possible to retrieve the version number of each member, which can then be used for version switching as needed.

Signed-off-by: yongjunhong <[email protected]>

codecov · 2025-09-21T09:15:36Z

Codecov Report

❌ Patch coverage is 5.26316% with 144 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.52%. Comparing base (df7172d) to head (d98da93).

Files with missing lines	Patch %	Lines
...scovery/registry/raft/RaftRegistryServiceImpl.java	5.14%	123 Missing and 6 partials ⚠️
...in/java/org/apache/seata/common/metadata/Node.java	0.00%	14 Missing ⚠️
.../org/apache/seata/common/util/Http2ClientUtil.java	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                2.x    #7586      +/-   ##
============================================
- Coverage     63.64%   63.52%   -0.13%     
- Complexity      990      991       +1     
============================================
  Files          1324     1324              
  Lines         50125    50233     +108     
  Branches       5920     5935      +15     
============================================
+ Hits          31901    31908       +7     
- Misses        15386    15483      +97     
- Partials       2838     2842       +4

Files with missing lines	Coverage Δ
.../org/apache/seata/common/util/Http2ClientUtil.java	`76.81% <50.00%> (ø)`
...in/java/org/apache/seata/common/metadata/Node.java	`40.33% <0.00%> (-5.38%)`	⬇️
...scovery/registry/raft/RaftRegistryServiceImpl.java	`17.72% <5.14%> (-5.17%)`	⬇️

... and 4 files with indirect coverage changes

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

funky-eyes · 2025-09-24T03:27:36Z

common/src/main/java/org/apache/seata/common/metadata/Node.java


+    @JsonIgnore
+    public boolean isHttp2Supported() {
+        String baseVersion = "2.6.0";


Could we use the Version class to compare versions instead?

funky-eyes · 2025-09-24T03:29:00Z

common/src/main/java/org/apache/seata/common/util/Http2ClientUtil.java

 import java.util.concurrent.TimeUnit;

-public class Http5ClientUtil {
+public class Http2ClientUtil {


Could we name it HttpClientUtil? It handles more than just HTTP/2 requests.

I think a factory class can be provided, for example, called HttpClientFactory, to obtain the specific client instance through the getClient method: For instance, if it is detected that http2 is available in the business system, use Http2ClientUtil; otherwise, use the original HttpClientUtil. However, this requires unifying the current apis of these two tool classes

However, recently I have been thinking about one point: whether the http2 function is supported or not not only depends on the business system's reliance on jar packages, but also on whether the TC end supports the http2 protocol. Currently, I'm considering whether the judgment logic for the latter can be directly placed in the getClient method of HttpClientFactory for judgment. If so, such an implementation would be more elegant. This is what I now solve this issue (#7561)

At first, what I considered was abstraction through an interface.
I thought it would be good to dynamically select the class at runtime via abstraction, but since the method return values of the http2 and http utility classes are different, that turned out to be impossible.

So for now, I also think a factory class is the best option. However, unifying the APIs of the two utility classes seems difficult at this point due to differences in their internal logic.

It might be a good idea to revisit this after you finish the work on #7561.
In the meantime, I’ll try to work on this PR over the weekend and wrap it up as quickly as possible.

funky-eyes · 2025-09-24T03:38:23Z

...ery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java

+
+    private static void executeHttp2WatchRequest(
+            String tcAddress, Map<String, String> param, Map<String, String> header, ResponseProcessor processor) {
+        Http2ClientUtil.doPost(


Since it was changed to HTTP/2 server push, will the watch thread started in startQueryMetadata that runs a while(true) loop become an infinite loop?

Could I ask you to provide some more explanation?
I'm not yet sure what exactly is causing concern.

Could I ask you to provide some more explanation? I'm not yet sure what exactly is causing concern.

With HTTP/2, configuration changes are delivered by the server via asynchronous pushes. Our current while(true) logic is based on HTTP/1.1 long-polling: the loop blocks synchronously waiting for the server response. Because HTTP/2 uses asynchronous server push, that while(true) no longer provides the blocking behavior and will instead continuously issue watch requests and metadata-fetch requests.

Copilot

Pull Request Overview

This PR integrates Raft consensus protocol with HTTP/2 support and implements protocol switching based on server version. The changes enable dynamic selection between HTTP/1.1 and HTTP/2 protocols depending on the Seata server version capabilities.

Adds HTTP/2 support with version-based protocol switching (2.6.0+ supports HTTP/2)
Refactors node selection and HTTP address resolution logic for better modularity
Updates method signatures to include Node parameter for protocol determination

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
RaftRegistryServiceImpl.java	Core implementation for protocol switching and HTTP/2 integration
RaftRegistryServiceImplTest.java	Updated test cases to match new method signatures with Node parameter
Http2ClientUtil.java	Renamed from Http5ClientUtil and updated method names for consistency
Http2ClientUtilTest.java	Updated test class name and method calls to match renamed utility
Node.java	Added version comparison logic to determine HTTP/2 support capability

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

common/src/main/java/org/apache/seata/common/metadata/Node.java

Copilot · 2025-09-24T03:39:35Z

...ery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java

+
+                return null;


This logic is inverted. When initAddresses is not empty, the method should process them and return a Node, not return null. The return null should be when initAddresses is empty.

Suggested change

return null;

InetSocketAddress inetSocketAddress =

initAddresses.get(ThreadLocalRandom.current().nextInt(initAddresses.size()));

// Construct a Node with the selected address. Other fields can be set to defaults or null.

Node node = new Node();

node.setAddress(NetUtil.toStringAddress(inetSocketAddress));

node.setCluster(clusterName);

node.setGroup(group);

return node;

Copilot · 2025-09-24T03:39:36Z

...raft/src/test/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImplTest.java

-                    () -> refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(), "127.0.0.1:8092"));
+
+            Node mockNode = mock(Node.class);
+            assertThrows(Exception.class, () -> refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode));


The method is being invoked with null as the instance parameter, but refreshToken is not a static method. It should be invoked on RaftRegistryServiceImpl.getInstance() instead of null.

Suggested change

assertThrows(Exception.class, () -> refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode));

assertThrows(Exception.class, () -> refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(), "127.0.0.1:8092", mockNode));

Copilot · 2025-09-24T03:39:36Z

...raft/src/test/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImplTest.java

+            metadata.put("external", externalEndpoints);
+            when(mockNode.getMetadata()).thenReturn(metadata);
+
+            refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode);


Same issue as above - the method should be invoked on RaftRegistryServiceImpl.getInstance() instead of null since refreshToken is not a static method.

Suggested change

refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode);

refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(), "127.0.0.1:8092", mockNode);

Copilot · 2025-09-24T03:39:36Z

...ery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java

                    } else if (statusCode == HttpStatus.SC_UNAUTHORIZED) {
                        if (StringUtils.isNotBlank(USERNAME) && StringUtils.isNotBlank(PASSWORD)) {
-                            refreshToken(tcAddress);
+                            refreshToken(tcAddress, selectedNode);


The refreshToken method signature has been updated to accept (String clusterName, Node selectedNode), but here it's being called with (String tcAddress, Node selectedNode). The first parameter should be the clusterName, not tcAddress.

Suggested change

refreshToken(tcAddress, selectedNode);

refreshToken(clusterName, selectedNode);

YvCeung · 2025-09-27T09:41:55Z

...ery-raft/src/main/java/org/apache/seata/discovery/registry/raft/RaftRegistryServiceImpl.java

+
+                    @Override
+                    public void onCancelled() {
+                        processor.process(null, new RetryableException("Request cancelled"));


RetryableException is triggered in the okhttp thread. The watch method does not throw this exception, which may result in it not being caught by the caller ?

YongGoose added 4 commits August 15, 2025 14:46

Rename method

1bff80d

Signed-off-by: yongjunhong <[email protected]>

Rename class

48a14a5

Signed-off-by: yongjunhong <[email protected]>

Add boolean method that verify node is support http2

6d5ee7d

Signed-off-by: yongjunhong <[email protected]>

support http2

43066d6

Signed-off-by: yongjunhong <[email protected]>

YongGoose commented Aug 15, 2025

View reviewed changes

YongGoose requested a review from funky-eyes August 15, 2025 13:26

YongGoose added 7 commits September 21, 2025 15:43

Merge branch '2.x' into feature/7544

ba7eb0a

[apache#7544] Implement http/2

b6eee18

Signed-off-by: yongjunhong <[email protected]>

[apache#7544] Remove korean

433b1b7

Signed-off-by: yongjunhong <[email protected]>

[apache#7544] Resolve pmd problem

698a629

Signed-off-by: yongjunhong <[email protected]>

[apache#7544] Remove comment

727c1ce

Signed-off-by: yongjunhong <[email protected]>

[apache#7544] Update TC

5bdfc01

Signed-off-by: yongjunhong <[email protected]>

[apache#7544] Add JsonIgnore annotation

223d9b9

Signed-off-by: yongjunhong <[email protected]>

funky-eyes added 2 commits September 22, 2025 16:52

Merge branch '2.x' into feature/7544

6f69b97

Merge branch '2.x' into feature/7544

5518d7f

funky-eyes reviewed Sep 24, 2025

View reviewed changes

funky-eyes requested a review from Copilot September 24, 2025 03:38

Copilot AI reviewed Sep 24, 2025

View reviewed changes

YvCeung reviewed Sep 27, 2025

View reviewed changes

YvCeung mentioned this pull request Oct 12, 2025

refactor:add the dependency probing logic of http and unify the apis of http1 and http2 #7684

Closed

2 tasks

Merge branch '2.x' into feature/7544

d98da93

-                return null;
+                InetSocketAddress inetSocketAddress =
+                        initAddresses.get(ThreadLocalRandom.current().nextInt(initAddresses.size()));
+                // Construct a Node with the selected address. Other fields can be set to defaults or null.
+                Node node = new Node();
+                node.setAddress(NetUtil.toStringAddress(inetSocketAddress));
+                node.setCluster(clusterName);
+                node.setGroup(group);
+                return node;

	assertThrows(Exception.class, () -> refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode));
	assertThrows(Exception.class, () -> refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(), "127.0.0.1:8092", mockNode));

	refreshTokenMethod.invoke(null, "127.0.0.1:8092", mockNode);
	refreshTokenMethod.invoke(RaftRegistryServiceImpl.getInstance(), "127.0.0.1:8092", mockNode);

	refreshToken(tcAddress, selectedNode);
	refreshToken(clusterName, selectedNode);

feature : Integrate Raft and Protocol Switching Based on Server Version #7586

Are you sure you want to change the base?

feature : Integrate Raft and Protocol Switching Based on Server Version #7586

Uh oh!

Conversation

YongGoose commented Aug 15, 2025

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

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

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Uh oh!

YongGoose commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

funky-eyes commented Aug 18, 2025

Uh oh!

codecov bot commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Sep 21, 2025 •

edited

Loading