dh-cloud commented on code in PR #2915:
URL: https://github.com/apache/tinkerpop/pull/2915#discussion_r1862881122
##########
gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/GremlinServer.java:
##########
@@ -146,6 +146,11 @@ public synchronized
CompletableFuture<ServerGremlinExecutor> start() throws Exce
b.childOption(ChannelOption.WRITE_BUFFER_WATER_MARK,
new WriteBufferWaterMark(settings.writeBufferLowWaterMark,
settings.writeBufferHighWaterMark));
b.childOption(ChannelOption.ALLOCATOR,
PooledByteBufAllocator.DEFAULT);
+ // Enable TCP Keep-Alive to detect if the remote peer is still
reachable.
+ // Keep-Alive sends periodic probes to check if the remote peer is
still active.
+ // If the remote peer is unreachable, the connection will be
closed, preventing resource leaks and
+ // avoiding the maintenance of stale connections.
+ b.childOption(ChannelOption.SO_KEEPALIVE, true);
Review Comment:
I have fully understood your point. For the requirement to disconnect
abnormal connections, I believe we can consider it from two dimensions:
When boolean supportsIdleMonitor() is false, i.e., idle connection detection
is disabled, we can enable SO_KEEPALIVE as a fallback to prevent broken
connections from remaining occupied.
When supportsIdleMonitor() is true, we need to improve the handling logic
for idle timeout connections.
In the TinkerPop master source code, I noticed that
settings.idleConnectionTimeout and settings.keepAliveInterval are both default
to 0. As a result, the new IdleStateHandler in AbstractChannelizer.class will
not have any effect.
```java
if (supportsIdleMonitor()) {
final int idleConnectionTimeout = (int) (settings.idleConnectionTimeout
/ 1000);
final int keepAliveInterval = (int) (settings.keepAliveInterval / 1000);
pipeline.addLast(new IdleStateHandler(idleConnectionTimeout,
keepAliveInterval, 0));
}
```
Setting specific values for idleConnectionTimeout and keepAliveInterval is
a complex task and has significant impact, as they control the idle threshold
for idle connections. However, different use cases have different requirements,
and it is challenging to provide a one-size-fits-all recommendation.
For this PR, I force pushed new commit implementing the fallback measure
only when boolean supportsIdleMonitor() is false.
I hope this helps! If you need any further adjustments, feel free to let me
know.
--
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]
