Skip to content

RATIS-2400. Support timeout and interrupt handling in GrpcClientRpc.#1342

Merged
szetszwo merged 1 commit intoapache:masterfrom
slfan1989:RATIS-2400
Feb 8, 2026
Merged

RATIS-2400. Support timeout and interrupt handling in GrpcClientRpc.#1342
szetszwo merged 1 commit intoapache:masterfrom
slfan1989:RATIS-2400

Conversation

@slfan1989
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds timeout and thread interruption handling support for synchronous gRPC client requests in GrpcClientRpc.

What is the link to the Apache JIRA

RATIS-2400. Support timeout and interrupt handling in GrpcClientRpc

How was this patch tested?

  • testGrpcClientRpcSyncTimeout: Verifies that synchronous requests correctly throw TimeoutIOException when the configured timeout is reached. Uses SimpleStateMachine4Testing.blockStartTransaction() to simulate a blocking scenario.

  • testGrpcClientRpcSyncCancelOnInterrupt: Verifies that when a request thread is interrupted, the request is properly cancelled and InterruptedIOException is thrown. Validates that the thread exits promptly after interruption.

@slfan1989
Copy link
Contributor Author

@szetszwo Could you please review this PR? Thank you very much!

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slfan1989 , thanks a lot for working on this!

Let's move the try-catch from the sendRequest(request) to sendRequest(request, proxy). Then, the code is simpler; See https://issues.apache.org/jira/secure/attachment/13080704/1342_review.patch

if (replyFuture.isCancelled()) {
requestObserver.onError(Status.CANCELLED
.withDescription(clientId + ": request #" + request.getCallId() + " cancelled")
.asRuntimeException());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use asException() instead of asRuntimeException().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your suggestion! I’ve already updated the code.

@szetszwo
Copy link
Contributor

szetszwo commented Feb 8, 2026

Let's move the try-catch from the sendRequest(request) to sendRequest(request, proxy). Then, the code is simpler; See https://issues.apache.org/jira/secure/attachment/13080704/1342_review.patch

@slfan1989 , Have you looked at the comment above?

@slfan1989
Copy link
Contributor Author

Let's move the try-catch from the sendRequest(request) to sendRequest(request, proxy). Then, the code is simpler; See https://issues.apache.org/jira/secure/attachment/13080704/1342_review.patch

@slfan1989 , Have you looked at the comment above?

@szetszwo Thank you for your suggestions! I’m updating this PR based on your feedback and will update it soon.

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

@szetszwo szetszwo merged commit b940b65 into apache:master Feb 8, 2026
16 checks passed
@slfan1989
Copy link
Contributor Author

+1 the change looks good.

@szetszwo Thanks for reviewing the code!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants