RATIS-2184. Improve TestRaftWithGrpc test stability #1177

jianghuazhu · 2024-11-12T07:52:44Z

What changes were proposed in this pull request?

Improve stability of TestRaftWithGrpc tests.

What is the link to the Apache JIRA

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

How was this patch tested?

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

jianghuazhu · 2024-11-12T07:59:57Z

@szetszwo @duongkame , can you help take it a look?
Thanks.

szetszwo

@jianghuazhu , thanks a lot for working on this! Please see the comments inlined.

szetszwo · 2024-11-13T00:35:40Z

ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderBase.java

+      ReferenceCountedObject<EntryWithData> entryWithData = null;
+      try {
+        entryWithData = getRaftLog().retainEntryWithData(next);
+        if (!buffer.offer(entryWithData.get())) {
+          entryWithData.release();
+          break;
+        }
+        offered.put(next, entryWithData);
+      } catch (Exception e){
+        if (entryWithData != null) {
+          entryWithData.release();
+        }


LogAppenderDaemon failed org.apache.ratis.server.raftlog.RaftLogIOException: Log entry not found: index = 4269 at org.apache.ratis.server.raftlog.segmented.SegmentedRaftLog.retainEntryWithData(SegmentedRaftLog.java:334) at org.apache.ratis.server.leader.LogAppenderBase.nextAppendEntriesRequest(LogAppenderBase.java:264)

For the above particular exception, this change won't help since, when retainEntryWithData(..) throws an exception, entryWithData must be null.

This change will help if other methods (e.g. get(), offer(..), put(..)) throw an exception. However, these methods throw only runtime exceptions/errors (e.g. OutOfMemoryError). We may not need to handle it.

szetszwo · 2024-11-13T00:38:01Z

pom.xml

@@ -643,7 +643,7 @@
            <enableProcessChecker>all</enableProcessChecker>
            <forkedProcessTimeoutInSeconds>600</forkedProcessTimeoutInSeconds>
            <!-- @argLine is filled by jacoco maven plugin. @{} means late evaluation -->
-            <argLine>-Xmx2g -XX:+HeapDumpOnOutOfMemoryError @{argLine}</argLine>
+            <argLine>-Xmx8g -XX:+HeapDumpOnOutOfMemoryError @{argLine}</argLine>


When turning on the advanced reference trace, it does need more memory. I recall that I did similar change for running with advanced reference trace.

szetszwo · 2024-11-13T00:38:44Z

pom.xml

-            <maxmem>2048m</maxmem>
+            <maxmem>4096m</maxmem>


Are you sure that it needs more memory for compilation?

jianghuazhu · 2024-11-18T12:24:12Z

@szetszwo , I updated some comments. In RATIS-2184.
Hope you can give some advice.

jianghuazhu · 2024-12-18T09:59:58Z

@szetszwo , I updated it.
From running the test, I found some problems. Here are some:

When cluster#shutdown() is called, there is no guarantee that RaftServerProxy will release all resources in advance.
The stack shows that LogSegment#readSegmentFile() is a very important entry.

Therefore, I improved a few points:

When cluster#shutdown() is called, try to release all resources as much as possible.
Where necessary, add some captures and call ReferenceCountedObject#release().
After many tests, no problems were found when running TestRaftWithGrpc.
ci : https://github.com/jianghuazhu/ratis/actions/runs/12390154687

szetszwo · 2024-12-18T18:05:09Z

... RaftServerProxy will release all resources in advance.

What resources? We need to fix if it is really the case.

After many tests, no problems were found ...

That's great! Let me start a build to repeating running many times.

szetszwo

@jianghuazhu , Thanks for the update! Please see the comments inlined.

szetszwo · 2024-12-18T18:07:21Z

ratis-grpc/src/main/java/org/apache/ratis/grpc/server/GrpcLogAppender.java

+    /*grpcServerMetrics.unregister();
+    CompletableFuture<LifeCycle.State> future = super.stopAsync();
+    if (appendLogRequestObserver != null) {
+      appendLogRequestObserver.stop();
+      appendLogRequestObserver = null;
+    }
+    return future;*/


This is a good change. Could you remove the commented code?

szetszwo · 2024-12-18T18:07:37Z

ratis-common/src/main/java/org/apache/ratis/util/ReferenceCountedLeakDetector.java

@@ -33,7 +33,6 @@
 public final class ReferenceCountedLeakDetector {
  private static final Logger LOG = LoggerFactory.getLogger(ReferenceCountedLeakDetector.class);
  // Leak detection is turned off by default.
-


Let's revert this whitespace change.

szetszwo · 2024-12-18T18:10:33Z

ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/LogSegment.java

-          toReturn.set(entryRef);
-        } else {
+        try {
+          final LogEntryProto entry = entryRef.retain();


entryRef.retain() should be called before the try-block. Otherwise, if it throws an exception, we will call release() without successfully retained.

szetszwo · 2024-12-18T18:11:40Z

ratis-server/src/main/java/org/apache/ratis/server/raftlog/segmented/SegmentedRaftLog.java

    try {
+      ref = retainLog(index);


Similar to the previous case, retainLog(index) should be called before the try-block.

szetszwo · 2024-12-18T18:16:30Z

Let me start a build to repeating running many times.

Started https://github.com/szetszwo/ratis/actions/runs/12398883445

jianghuazhu · 2024-12-19T10:13:52Z

Let me start a build to repeating running many times.

Started https://github.com/szetszwo/ratis/actions/runs/12398883445

Sorry, there seem to be some errors or omissions that have not been discovered.

szetszwo · 2024-12-19T14:08:37Z

@jianghuazhu , The 10x100 build timed out. Let's retry with 10x10:
https://github.com/szetszwo/ratis/actions/runs/12414246844

szetszwo · 2024-12-19T18:53:31Z

@jianghuazhu , compared with the master, your branch does have improved the success rate

master Total failures: 42 ( https://github.com/apache/ratis/actions/runs/12416764228/job/34669317055 )
your branch failures: 20 ( https://github.com/szetszwo/ratis/actions/runs/12414246844/job/34661434032 )

Could you clean up the code? We may merge it first and have some further improvement in a separated JIRA.

jianghuazhu · 2024-12-20T04:04:15Z

@jianghuazhu , compared with the master, your branch does have improved the success rate

master Total failures: 42 ( https://github.com/apache/ratis/actions/runs/12416764228/job/34669317055 )

your branch failures: 20 ( https://github.com/szetszwo/ratis/actions/runs/12414246844/job/34661434032 )

Could you clean up the code? We may merge it first and have some further improvement in a separated JIRA.

Thanks @szetszwo .
I have updated it.

szetszwo reviewed Nov 13, 2024

View reviewed changes

RATIS-2184. Improve TestRaftWithGrpc test stability

415799f

jianghuazhu force-pushed the RATIS-2184 branch from 6e941ad to 415799f Compare December 18, 2024 09:32

szetszwo reviewed Dec 18, 2024

View reviewed changes

Cleaned up some code.

cb607bd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RATIS-2184. Improve TestRaftWithGrpc test stability #1177

RATIS-2184. Improve TestRaftWithGrpc test stability #1177

jianghuazhu commented Nov 12, 2024

jianghuazhu commented Nov 12, 2024

szetszwo left a comment

szetszwo Nov 13, 2024

szetszwo Nov 13, 2024

szetszwo Nov 13, 2024

jianghuazhu commented Nov 18, 2024

jianghuazhu commented Dec 18, 2024 •

edited

Loading

szetszwo commented Dec 18, 2024

szetszwo left a comment

szetszwo Dec 18, 2024

szetszwo Dec 18, 2024

szetszwo Dec 18, 2024

szetszwo Dec 18, 2024

szetszwo commented Dec 18, 2024

jianghuazhu commented Dec 19, 2024

szetszwo commented Dec 19, 2024

szetszwo commented Dec 19, 2024

jianghuazhu commented Dec 20, 2024

RATIS-2184. Improve TestRaftWithGrpc test stability #1177

Are you sure you want to change the base?

RATIS-2184. Improve TestRaftWithGrpc test stability #1177

Conversation

jianghuazhu commented Nov 12, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

jianghuazhu commented Nov 12, 2024

szetszwo left a comment

Choose a reason for hiding this comment

szetszwo Nov 13, 2024

Choose a reason for hiding this comment

szetszwo Nov 13, 2024

Choose a reason for hiding this comment

szetszwo Nov 13, 2024

Choose a reason for hiding this comment

jianghuazhu commented Nov 18, 2024

jianghuazhu commented Dec 18, 2024 • edited Loading

szetszwo commented Dec 18, 2024

szetszwo left a comment

Choose a reason for hiding this comment

szetszwo Dec 18, 2024

Choose a reason for hiding this comment

szetszwo Dec 18, 2024

Choose a reason for hiding this comment

szetszwo Dec 18, 2024

Choose a reason for hiding this comment

szetszwo Dec 18, 2024

Choose a reason for hiding this comment

szetszwo commented Dec 18, 2024

jianghuazhu commented Dec 19, 2024

szetszwo commented Dec 19, 2024

szetszwo commented Dec 19, 2024

jianghuazhu commented Dec 20, 2024

jianghuazhu commented Dec 18, 2024 •

edited

Loading