MAPREDUCE-7475. Fixed non-idempotent unit tests #6785
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
steveloughran
requested changes
May 1, 2024
...duce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/TestNewCombinerGrouping.java
Outdated
Show resolved
Hide resolved
...duce-client-jobclient/src/test/java/org/apache/hadoop/mapreduce/TestNewCombinerGrouping.java
Outdated
Show resolved
Hide resolved
|
will merge once the build completes. |
ayushtkn
reviewed
May 4, 2024
There was a problem hiding this comment.
why don't we just cleanup the testDir in After? If you run this test a couple of times and see the test_root_dir
ayushsaxena@ayushsaxena hadoop % ls -l /Users/ayushsaxena/code/hadoop/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/target/test-dir/
total 0
drwxr-xr-x@ 4 ayushsaxena staff 128 May 4 12:13 22d6c31d-6195-41d5-b3a0-835940271ce5
drwxr-xr-x@ 4 ayushsaxena staff 128 May 4 12:12 6a754c2e-3c13-4b85-94fa-8a654ebdf936
drwxr-xr-x@ 4 ayushsaxena staff 128 May 4 12:16 ab9a29a6-8210-476e-8912-e6d9fdcd40e3
drwxr-xr-x@ 4 ayushsaxena staff 128 May 4 12:15 e22b7045-cefa-4979-b94c-1a5edb2eea73
ayushsaxena@ayushsaxena hadoop %
it does have all those directories, MR tests don't delete the testDir in the pom either.
Why don't we try something like this
diff --git a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestOldCombinerGrouping.java b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestOldCombinerGrouping.java
index 046c2d37eed9..d6b4952f9d6a 100644
--- a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestOldCombinerGrouping.java
+++ b/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-jobclient/src/test/java/org/apache/hadoop/mapred/TestOldCombinerGrouping.java
@@ -18,11 +18,16 @@
package org.apache.hadoop.mapred;
+import org.junit.After;
import org.junit.Assert;
+
+import org.apache.hadoop.fs.FileUtil;
import org.apache.hadoop.fs.Path;
import org.apache.hadoop.io.LongWritable;
import org.apache.hadoop.io.RawComparator;
import org.apache.hadoop.io.Text;
+import org.apache.hadoop.test.GenericTestUtils;
+
import org.junit.Test;
import java.io.BufferedReader;
@@ -34,12 +39,9 @@
import java.util.HashSet;
import java.util.Iterator;
import java.util.Set;
-import java.util.UUID;
public class TestOldCombinerGrouping {
- private static String TEST_ROOT_DIR = new File(System.getProperty(
- "test.build.data", "build/test/data"), UUID.randomUUID().toString())
- .getAbsolutePath();
+ private static File TEST_ROOT_DIR = GenericTestUtils.getRandomizedTestDir();
public static class Map implements
Mapper<LongWritable, Text, Text, LongWritable> {
@@ -117,9 +119,14 @@ public int compare(Text o1, Text o2) {
}
+ @After
+ public void cleanup() {
+ FileUtil.fullyDelete(TEST_ROOT_DIR);
+ }
+
@Test
public void testCombiner() throws Exception {
- if (!new File(TEST_ROOT_DIR).mkdirs()) {
+ if (!TEST_ROOT_DIR.mkdirs()) {
throw new RuntimeException("Could not create test dir: " + TEST_ROOT_DIR);
}
File in = new File(TEST_ROOT_DIR, "input");
but I am kind of ok with the current impl as well if @steveloughran is convinced
|
@ayushtkn Thanks for the review! I agree that its better to cleanup the test directory in |
|
@kaiyaok2 Can #6790 and #6785 be fixed together? |
|
@slfan1989 Yes. I merged all code changes in #6790 to this PR (#6785) |
goiri
approved these changes
May 5, 2024
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
ayushtkn
approved these changes
May 14, 2024
There was a problem hiding this comment.
yep, mvn clean should do the cleanup here.
The last few builds are from the windows precommit which doesn't run the tests, It got enabled again & it doesn't let the normal builds run this time I believe.
I have manually started the normal build for this to get the test results:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-6785/7/
If the tests pass, will hit the merge button
|
💔 -1 overall
This message was automatically generated. |
|
We have the test results, everything is green, the checkstyle alarm is for the existing code, not introduced as part of this PR but I think since we are touching it now, maybe we can fix that as well it is just the variable name change, maybe changing to |
@ayushtkn Fixed. Thanks for the message |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
thanks! merged to trunk. Can you do a cherrypick to branch-3.4 and push that up as a PR too so that yetus can validate the backport. Then we can merge it in there too |
kaiyaok2
added a commit
to kaiyaok2/hadoop
that referenced
this pull request
May 18, 2024
Contributed by Kaiyao Ke
|
@steveloughran Done in #6837 Please lmk if I shall do the same for #6793 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
This PR fixes unit tests that are not idempotent and fail upon repeated execution within the same JVM instance due to self-induced state pollution. The tests shall be fixed as unit tests shall be idempotent, and it shall be good to clean the state pollution so that potential newly introduced tests do not fail in the future due to the shared state polluted by these tests.
Overview & Proposed Fix of all non-idempotent unit tests in the MapReduce Module
The following tests try to make the directory
TEST_ROOT_DIRand write to it. The tests do not clean up (remove) the directory after execution. Therefore, in the second execution,TEST_ROOT_DIRwould already exist and the exceptionCould not create test dirwould be thrown.Sample Failure Message (in the 2nd run of the test):
The fix is done by fully deleting the test directory after test execution.
The following two tests below do not reset
NotificationServlet.counter, so repeated runs throw assertion failures due to accumulation.Fixed by resetting
NotificationServlet.counterandNotificationServlet.failureCounterto 0 after test execution.The following test does not remove the key
AMParams.ATTEMPT_STATE, so repeated runs of the test will not be missing the attempt-state at all:Fixed by removing
AMParams.ATTEMPT_STATEat the end of the test.The following test fully deletes
TEST_ROOT_DIRafter execution, so repeated runs will throw aDiskErrorException:Fixed by checking if
TEST_ROOT_DIRexists before test execution. Make the directory if not.The following test does not restore the static variable
statusUpdateTimesafter execution, so consecutive runs throwsAssertionError:Fixed by resetting
statusUpdateTimesto 0 before test executionHow was this patch tested?
After the patch, rerunning the tests in the same JVM does not produce any exceptions.