szetszwo commented on code in PR #1368:
URL: https://github.com/apache/ratis/pull/1368#discussion_r3149320107
##########
ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java:
##########
@@ -366,16 +374,46 @@ void onNext(AppendEntriesRequestProto proto)
while (!stream.isReady() && running) {
sleep(waitForReady, isHeartBeat);
}
+ if (!running) {
+ return;
+ }
stream.onNext(proto);
}
- void stop() {
+ void stop(Event event) {
running = false;
+ if (event == Event.COMPLETE) {
+ onCompleted();
+ } else {
+ cancelStream("stop due to " + event);
Review Comment:
We have the following call sequence:
- AppendLogResponseHandler.onError(..) / onCompleted() -> resetClient(..) ->
stop()
In the [gRPC
example](https://grpc.io/docs/languages/java/basics/#client-side-streaming-rpc),
- onCompleted() implementation will response onCompleted() but
- onError(Throwable t) only calls logger.log(Level.WARNING, ...)
So, I think stop() should not call cancelStream(..)
##########
ratis-test/src/test/java/org/apache/ratis/grpc/TestLogAppenderWithGrpc.java:
##########
@@ -148,4 +159,207 @@ private void
runTestRestartLogAppender(MiniRaftClusterWithGrpc cluster) throws E
Assertions.assertTrue(newleaderMetrics.getRegistry().counter(counter).getCount()
>= 1L);
}
}
+
+ /**
+ * Verify that old LogAppender instances are properly cleaned up (gRPC
streams terminated)
+ * after restartLogAppenders. Without the fix, gRPC holds references to
unterminated
+ * stream response handlers, preventing old LogAppender instances from being
collected.
+ */
+ @ParameterizedTest
+ @MethodSource("data")
+ public void testLogAppenderStreamCleanupOnRestart(Boolean separateHeartbeat)
throws Exception {
Review Comment:
I tried to revert GrpcLogAppender and GrpcServerProtocolService (except for
GRPC_SERVER_HANDLE_ERROR). Both testLogAppenderStreamCleanupOnRestart and
testFollowerHandleErrorCleanup can pass. So, they probably cannot test the
code change.
--
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]
