Copilot commented on code in PR #7826:
URL: https://github.com/apache/incubator-seata/pull/7826#discussion_r2612671833
##########
server/pom.xml:
##########
@@ -301,7 +301,6 @@
<dependency>
<groupId>com.squareup.okhttp3</groupId>
<artifactId>okhttp</artifactId>
Review Comment:
The okhttp3 dependency scope has been changed from 'test' to compile
(default), but the dependency appears to only be used in test code
(ClusterControllerTest.java). This unnecessarily includes okhttp3 as a runtime
dependency in production builds. The scope should remain as 'test' unless there
is production code that requires it.
```suggestion
<artifactId>okhttp</artifactId>
<scope>test</scope>
```
##########
server/src/test/java/org/apache/seata/server/controller/ClusterControllerTest.java:
##########
@@ -152,7 +150,57 @@ public void run() {
}
@Test
- @Order(4)
+ @Order(5)
+ void watch_withHttp2() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+
+ Map<String, String> headers = new HashMap<>();
+ headers.put(HTTP.CONTENT_TYPE,
ContentType.APPLICATION_FORM_URLENCODED.getMimeType());
+
+ Map<String, String> params = new HashMap<>();
+ params.put("default-test", "1");
+
+ Thread thread = new Thread(new Runnable() {
+ @Override
+ public void run() {
+ try {
+ Thread.sleep(2000);
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
+ ((ApplicationEventPublisher)
ObjectHolder.INSTANCE.getObject(OBJECT_KEY_SPRING_APPLICATION_CONTEXT))
+ .publishEvent(new ClusterChangeEvent(this,
"default-test", 2, true));
+ }
Review Comment:
The test uses an anonymous Runnable class with explicit override annotation,
which is verbose. Consider using a lambda expression for cleaner code: `Thread
thread = new Thread(() -> { ... });` This improves readability and follows
modern Java coding practices.
```suggestion
Thread thread = new Thread(() -> {
try {
Thread.sleep(2000);
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
((ApplicationEventPublisher)
ObjectHolder.INSTANCE.getObject(OBJECT_KEY_SPRING_APPLICATION_CONTEXT))
.publishEvent(new ClusterChangeEvent(this,
"default-test", 2, true));
```
##########
server/src/main/java/org/apache/seata/server/cluster/manager/ClusterWatcherManager.java:
##########
@@ -107,22 +111,33 @@ private void sendWatcherResponse(Watcher<HttpContext>
watcher, HttpResponseStatu
return;
}
ChannelHandlerContext ctx = context.getContext();
+ if (!ctx.channel().isActive()) {
+ logger.warn(
+ "Netty channel is not active for watcher on group {},
cannot send response.", watcher.getGroup());
+ return;
+ }
+
if (!context.isHttp2()) {
- if (ctx.channel().isActive()) {
- HttpResponse response =
- new DefaultFullHttpResponse(HttpVersion.HTTP_1_1,
nettyStatus, Unpooled.EMPTY_BUFFER);
- response.headers().set(HttpHeaderNames.CONTENT_LENGTH, 0);
+ // HTTP/1.1 response
+ HttpResponse response =
+ new DefaultFullHttpResponse(HttpVersion.HTTP_1_1,
nettyStatus, Unpooled.EMPTY_BUFFER);
+ response.headers().set(HttpHeaderNames.CONTENT_LENGTH, 0);
- if (!context.isKeepAlive()) {
-
ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE);
- } else {
- ctx.writeAndFlush(response);
- }
+ if (!context.isKeepAlive()) {
+
ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE);
} else {
- logger.warn(
- "Netty channel is not active for watcher on group {},
cannot send response.",
- watcher.getGroup());
+ ctx.writeAndFlush(response);
}
+ } else {
+ // HTTP/2 response (h2c support)
+ // Send headers frame
+ Http2Headers headers = new
DefaultHttp2Headers().status(nettyStatus.codeAsText());
+ headers.set(HttpHeaderNames.CONTENT_LENGTH, "0");
+ ctx.write(new DefaultHttp2HeadersFrame(headers));
+
+ // Send empty data frame with endStream=true to close the stream
+ ctx.write(new DefaultHttp2DataFrame(Unpooled.EMPTY_BUFFER, true));
Review Comment:
The HTTP/2 response handling does not include error handling for write
failures. Unlike the HTTP/1.1 path which can use ChannelFutureListener.CLOSE
for non-keepalive connections, the HTTP/2 path uses separate write operations
without checking their success. Consider adding listeners to the write
operations to handle failures or at least log them, similar to how HTTP/1.1
handles the writeAndFlush result.
```suggestion
ctx.write(new DefaultHttp2DataFrame(Unpooled.EMPTY_BUFFER, true))
.addListener(future -> {
if (!future.isSuccess()) {
logger.warn("Failed to write HTTP/2 response for
watcher on group {}: {}", watcher.getGroup(), future.cause().toString());
}
});
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
