RATIS-1977. Remove Junit 4 dependencies. #1269

slfan1989 · 2025-05-22T22:06:40Z

What changes were proposed in this pull request?

We will try to remove the junit4 dependency.

What is the link to the Apache JIRA

JIRA: RATIS-1977. Remove Junit 4 dependencies.

How was this patch tested?

mvn clean test.

slfan1989 · 2025-05-22T22:12:29Z

ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java


  static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader)
      throws Exception {
-    return changeLeader(cluster, oldLeader, AssumptionViolatedException::new);


This part needs some clarification: we've replaced org.junit.AssumptionViolatedException with org.opentest4j.TestAbortedException. The reason for this change is that JUnit 5 internally uses TestAbortedException when handling assumptions via org.junit.jupiter.api.Assumptions.assumeTrue. Given this, I believe it's appropriate to adopt TestAbortedException directly in our implementation.

Let's use Assumptions.abort(..) since it is a public API.

diff --git a/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java b/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java index c9db9933b2..d0641e39c4 100644 --- a/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java +++ b/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java @@ -44,7 +44,8 @@ import org.apache.ratis.util.JavaUtils; import org.apache.ratis.util.Preconditions; import org.apache.ratis.util.ProtoUtils; import org.apache.ratis.util.TimeDuration; -import org.opentest4j.TestAbortedException; +import org.apache.ratis.util.function.CheckedConsumer; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Assertions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -479,18 +480,18 @@ public interface RaftTestUtil { static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader) throws Exception { - return changeLeader(cluster, oldLeader, TestAbortedException::new); + return changeLeader(cluster, oldLeader, Assumptions::abort); } - static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader, Function<String, Exception> constructor) - throws Exception { + static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader, + CheckedConsumer<String, Exception> failToChangeLeaderHandler) throws Exception { final String name = JavaUtils.getCallerStackTraceElement().getMethodName() + "-changeLeader"; cluster.setBlockRequestsFrom(oldLeader.toString(), true); try { return JavaUtils.attemptRepeatedly(() -> { final RaftPeerId newLeader = waitForLeader(cluster).getId(); if (newLeader.equals(oldLeader)) { - throw constructor.apply("Failed to change leader: newLeader == oldLeader == " + oldLeader); + failToChangeLeaderHandler.accept("Failed to change leader: newLeader == oldLeader == " + oldLeader); } LOG.info("Changed leader from " + oldLeader + " to " + newLeader); return newLeader; diff --git a/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java b/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java index f35626894f..25caa9d06e 100644 --- a/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java +++ b/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java @@ -184,7 +184,7 @@ public abstract class LeaderElectionTests<CLUSTER extends MiniRaftCluster> void runTestChangeLeader(MiniRaftCluster cluster) throws Exception { RaftPeerId leader = RaftTestUtil.waitForLeader(cluster).getId(); for(int i = 0; i < 10; i++) { - leader = RaftTestUtil.changeLeader(cluster, leader, IllegalStateException::new); + leader = RaftTestUtil.changeLeader(cluster, leader, Assertions::fail); } }

@szetszwo Thanks for the review! I’ve made the updates as suggested.

slfan1989 · 2025-05-22T22:34:55Z

@szetszwo @adoroszlai RATIS-1977 has passed all checks, indicating that Ratis is now well-positioned to remove JUnit4. This PR also includes a few additional code changes, but they are relatively minor. To save the reviewers' time, I have included them in this submission as well. Could you help review this PR? Thank you very much!

szetszwo

@slfan1989 , thanks a lot for completing the junit 5 migration! It is really helpful.

Please the comment regarding to TestAbortedException.

szetszwo · 2025-05-23T02:25:57Z

ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java


  static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader)
      throws Exception {
-    return changeLeader(cluster, oldLeader, AssumptionViolatedException::new);


Let's use Assumptions.abort(..) since it is a public API.

diff --git a/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java b/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java index c9db9933b2..d0641e39c4 100644 --- a/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java +++ b/ratis-server/src/test/java/org/apache/ratis/RaftTestUtil.java @@ -44,7 +44,8 @@ import org.apache.ratis.util.JavaUtils; import org.apache.ratis.util.Preconditions; import org.apache.ratis.util.ProtoUtils; import org.apache.ratis.util.TimeDuration; -import org.opentest4j.TestAbortedException; +import org.apache.ratis.util.function.CheckedConsumer; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Assertions; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -479,18 +480,18 @@ public interface RaftTestUtil { static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader) throws Exception { - return changeLeader(cluster, oldLeader, TestAbortedException::new); + return changeLeader(cluster, oldLeader, Assumptions::abort); } - static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader, Function<String, Exception> constructor) - throws Exception { + static RaftPeerId changeLeader(MiniRaftCluster cluster, RaftPeerId oldLeader, + CheckedConsumer<String, Exception> failToChangeLeaderHandler) throws Exception { final String name = JavaUtils.getCallerStackTraceElement().getMethodName() + "-changeLeader"; cluster.setBlockRequestsFrom(oldLeader.toString(), true); try { return JavaUtils.attemptRepeatedly(() -> { final RaftPeerId newLeader = waitForLeader(cluster).getId(); if (newLeader.equals(oldLeader)) { - throw constructor.apply("Failed to change leader: newLeader == oldLeader == " + oldLeader); + failToChangeLeaderHandler.accept("Failed to change leader: newLeader == oldLeader == " + oldLeader); } LOG.info("Changed leader from " + oldLeader + " to " + newLeader); return newLeader; diff --git a/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java b/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java index f35626894f..25caa9d06e 100644 --- a/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java +++ b/ratis-server/src/test/java/org/apache/ratis/server/impl/LeaderElectionTests.java @@ -184,7 +184,7 @@ public abstract class LeaderElectionTests<CLUSTER extends MiniRaftCluster> void runTestChangeLeader(MiniRaftCluster cluster) throws Exception { RaftPeerId leader = RaftTestUtil.waitForLeader(cluster).getId(); for(int i = 0; i < 10; i++) { - leader = RaftTestUtil.changeLeader(cluster, leader, IllegalStateException::new); + leader = RaftTestUtil.changeLeader(cluster, leader, Assertions::fail); } }

szetszwo

@slfan1989 , Thanks for the update! A few comments inlined.

szetszwo · 2025-05-23T02:48:47Z

pom.xml

    <junit-bom.version>5.11.2</junit-bom.version>
+    <mockito.version>4.3.1</mockito.version>


Let's also update the versions

junit: 5.12.2

mockito: 4.11.0 (upgrading to mockito 5 seems not easy: https://davidvlijmincx.com/posts/upgrade-to-mockito-5/)

szetszwo · 2025-05-23T02:49:36Z

pom.xml

      <dependency>
        <groupId>org.apache.tomcat</groupId>
        <artifactId>annotations-api</artifactId>
-        <version>6.0.53</version>
+        <version>${tomcat.version}</version>
        <scope>provided</scope>
      </dependency>


tomcat dependency can be removed.

diff --git a/pom.xml b/pom.xml index af14e426f2..6fca9e66d7 100644 --- a/pom.xml +++ b/pom.xml @@ -435,12 +435,6 @@ <artifactId>mockito-core</artifactId> <version>4.3.1</version> </dependency> - <dependency> - <groupId>org.apache.tomcat</groupId> - <artifactId>annotations-api</artifactId> - <version>6.0.53</version> - <scope>provided</scope> - </dependency> </dependencies> </dependencyManagement> diff --git a/ratis-proto/pom.xml b/ratis-proto/pom.xml index e3342f84ba..801633585d 100644 --- a/ratis-proto/pom.xml +++ b/ratis-proto/pom.xml @@ -175,9 +175,5 @@ <groupId>org.apache.ratis</groupId> <artifactId>ratis-thirdparty-misc</artifactId> </dependency> - <dependency> - <groupId>org.apache.tomcat</groupId> - <artifactId>annotations-api</artifactId> - </dependency> </dependencies> </project>

We have removed the dependency on Tomcat, but we currently cannot provide the javax.annotation.Generated annotation. As a temporary workaround, we included javax.annotation:javax.annotation-api:1.3.2. This approach is based on the discussion in grpc-java issue #9179. According to the latest commit (PR #12080), the issue has been fixed and merged two days ago. It is expected to be resolved in the next release of grpc-java.

I have an idea: we could temporarily keep the current Tomcat dependency, and remove it after the next grpc release includes the fix. Would this approach be feasible?

@slfan1989 , in this case, we should use jakarta.annotation-api since the license of javax.annotation-api cannot be used in Apache.

Thank you for the clarification! I have switched the dependency to jakarta.annotation-api, and the CI tests are currently passing. For now, we can only use version 1.3.5, as the package name changes to jakarta.annotation starting from version 2.0.0.

szetszwo · 2025-05-23T02:55:46Z

ratis-common/src/test/java/org/apache/ratis/BaseTest.java

        + "." + (method == null? null : method.getName());
  }

-  // @Before annotation is retained to support junit 4 tests.


Let's also remove the other two junit 4 comments:

+++ b/ratis-common/src/test/java/org/apache/ratis/BaseTest.java @@ -72,8 +72,6 @@ public abstract class BaseTest { } } - // TODO: Junit 4 reference should be removed once all the unit tests are migrated to Junit 5. - private String testCaseName; @BeforeEach @@ -133,7 +131,6 @@ public abstract class BaseTest { } public File getTestDir() { - // This will work for both junit 4 and 5. return new File(getClassTestDir(), testCaseName); }

Thank you for pointing out these issues! I have made the changes as suggested.

szetszwo

+1 the change looks good.

slfan1989 · 2025-05-23T08:19:00Z

@szetszwo Thank you very much for reviewing the code! After this task is completed, we have a few other issues we'd like to further discuss with you. In our production environment, the Ozone cluster has grown to over 3,000 machines. Due to the large volume of metadata and high write concurrency, OM followers often fail to keep up with the leader’s index, resulting in frequent snapshot fetches from the leader. We are actively exploring optimization solutions and will continue to keep in touch with you.

szetszwo · 2025-05-23T15:25:39Z

@slfan1989 , sure, please feel free to let me know if there is anything I could help!

Also, thanks a lot for your contribution on Ratis and Ozone!

kerneltime · 2025-05-23T15:51:58Z

Do you'll use NVMe for OM disks?

slfan1989 · 2025-05-23T21:59:31Z

@szetszwo @kerneltime Thank you both for your responses. I will create a JIRA in RATIS for further follow-up and will summarize the background and the issues we’re currently facing. Our OM is deployed on servers equipped with two NVMe disks, configured with RAID 1 for data protection. However, this setup may lead to slightly reduced write performance compared to using a single NVMe disk.

slfan1989 added 2 commits May 23, 2025 05:49

RATIS-1977. Remove Junit 4 dependencies.

5ec6bcc

RATIS-1977. Remove Junit 4 dependencies.

c8b6f2c

slfan1989 commented May 22, 2025

View reviewed changes

szetszwo reviewed May 23, 2025

View reviewed changes

RATIS-1977. Improve Code.

77e8c0b

szetszwo reviewed May 23, 2025

View reviewed changes

slfan1989 added 3 commits May 23, 2025 11:16

RATIS-1977. Improve Code.

8749e5f

RATIS-1977. Improve Code.

c00a0aa

RATIS-1977. Use jakarta annotation.

4801472

szetszwo approved these changes May 23, 2025

View reviewed changes

szetszwo merged commit c56ce42 into apache:master May 23, 2025
15 checks passed

szetszwo pushed a commit to szetszwo/ratis that referenced this pull request May 23, 2025

RATIS-1977. Remove Junit 4 dependencies. (apache#1269)

371f68c

		<junit-bom.version>5.11.2</junit-bom.version>
		<mockito.version>4.3.1</mockito.version>

RATIS-1977. Remove Junit 4 dependencies. #1269

RATIS-1977. Remove Junit 4 dependencies. #1269

Uh oh!

Conversation

slfan1989 commented May 22, 2025

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slfan1989 commented May 22, 2025

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slfan1989 May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

slfan1989 commented May 23, 2025

Uh oh!

szetszwo commented May 23, 2025

Uh oh!

kerneltime commented May 23, 2025

Uh oh!

slfan1989 commented May 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

slfan1989 May 23, 2025 •

edited

Loading