Skip to content

Conversation

@jianghuazhu
Copy link
Contributor

What changes were proposed in this pull request?

In Ratis, some of the messaging for Objects.requireNonNull() is not clear enough. The purpose of this PR is to try to fix that.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/RATIS-2182

How was this patch tested?

ci:
https://github.com/jianghuazhu/ratis/actions/runs/11590363184

@jianghuazhu
Copy link
Contributor Author

@szetszwo , can you help review this PR?
Thanks.

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
Copy link
Contributor

szetszwo commented Oct 30, 2024

@jianghuazhu , thanks a lot for working on this!

TestRaftWithGrpc failed in the previous run. Let's rerun it.

BTW, please see if you have time to fix the remaining zero-copy bugs. We may enable advanced tracing as below and run the test until it fails. Then, check the retain-release trace to see why an object is NOT released properly.

+++ b/ratis-grpc/src/test/java/org/apache/ratis/grpc/MiniRaftClusterWithGrpc.java
@@ -52,7 +52,7 @@ public class MiniRaftClusterWithGrpc extends MiniRaftCluster.RpcBase {
 
   static {
     // TODO move it to MiniRaftCluster for detecting non-gRPC cases
-    ReferenceCountedLeakDetector.enable(false);
+    ReferenceCountedLeakDetector.enable(true);
   }

@jianghuazhu
Copy link
Contributor Author

@jianghuazhu , thanks a lot for working on this!

TestRaftWithGrpc failed in the previous run. Let's rerun it.

BTW, please see if you have time to fix the remaining zero-copy bugs. We may enable advanced tracing as below and rerun the test until it fails. Then, check the retain-release trace to see why an object is released properly.

+++ b/ratis-grpc/src/test/java/org/apache/ratis/grpc/MiniRaftClusterWithGrpc.java
@@ -52,7 +52,7 @@ public class MiniRaftClusterWithGrpc extends MiniRaftCluster.RpcBase {
 
   static {
     // TODO move it to MiniRaftCluster for detecting non-gRPC cases
-    ReferenceCountedLeakDetector.enable(false);
+    ReferenceCountedLeakDetector.enable(true);
   }

Thanks @szetszwo for the comment and review.
I will try to fix the TestRaftWithGrpc test failure and I will create a new jira later.

@szetszwo szetszwo merged commit 237b7c0 into apache:master Oct 31, 2024
szetszwo pushed a commit to szetszwo/ratis that referenced this pull request Mar 30, 2025
szetszwo pushed a commit to szetszwo/ratis that referenced this pull request May 23, 2025
szetszwo pushed a commit to szetszwo/ratis that referenced this pull request May 23, 2025
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