From 0413eb3cd73f64223ebbae597af73b92cb751e28 Mon Sep 17 00:00:00 2001 From: zhangshuyan Date: Wed, 14 Jun 2023 11:11:09 +0800 Subject: [PATCH 1/3] HDFS-17049. EC: Fix duplicate block group IDs generated by SequentialBlockGroupIdGenerator. --- .../SequentialBlockGroupIdGenerator.java | 2 +- .../TestSequentialBlockGroupId.java | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java index 7a522730e50f1c..f2e989ab2c5d07 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java @@ -51,7 +51,7 @@ public class SequentialBlockGroupIdGenerator extends SequentialNumber { } @Override // NumberGenerator - public long nextValue() { + synchronized public long nextValue() { skipTo((getCurrentValue() & ~BLOCK_GROUP_INDEX_MASK) + MAX_BLOCKS_IN_GROUP); // Make sure there's no conflict with existing random block IDs final Block b = new Block(getCurrentValue()); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java index 25b2a028835781..c1d83736461ab7 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java @@ -22,11 +22,15 @@ import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.spy; import java.io.IOException; +import java.util.ArrayList; +import java.util.HashSet; import java.util.List; +import java.util.Set; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -172,6 +176,41 @@ public void testTriggerBlockGroupIdCollision() throws IOException { } } + /** + * Test that the values generated by blockGroup ID generator are unique, + * even if they are generated concurrently. + * @throws Exception + */ + @Test + public void testBlockGroupIdThreadSafety() throws Exception { + List> blockIds = new ArrayList<>(); + List threads = new ArrayList<>(); + for (int i = 0; i < 20; i++) { + blockIds.add(new ArrayList<>()); + threads.add(new Thread(() -> { + for (int j = 0; j < 1000; j++) { + long next = blockGrpIdGenerator.nextValue(); + blockIds.get(j).add(next); + } + })); + } + for (Thread t : threads) { + t.start(); + } + for (Thread t : threads) { + t.join(); + } + Set allBlockIds = new HashSet<>(); + for (List set : blockIds) { + for (long id : set) { + if (allBlockIds.contains(id)) { + fail("Same block group id is generated!"); + } + allBlockIds.add(id); + } + } + } + /** * Test that collisions in the blockGroup ID when the id is occupied by legacy * block. From 927cb4b5a3cc05b8d7581fe5b7aaf2f08a8f7282 Mon Sep 17 00:00:00 2001 From: zhangshuyan Date: Wed, 14 Jun 2023 16:13:47 +0800 Subject: [PATCH 2/3] feedback review. --- .../blockmanagement/SequentialBlockGroupIdGenerator.java | 2 +- .../server/blockmanagement/TestSequentialBlockGroupId.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java index f2e989ab2c5d07..ba539fdf674802 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/SequentialBlockGroupIdGenerator.java @@ -51,7 +51,7 @@ public class SequentialBlockGroupIdGenerator extends SequentialNumber { } @Override // NumberGenerator - synchronized public long nextValue() { + public synchronized long nextValue() { skipTo((getCurrentValue() & ~BLOCK_GROUP_INDEX_MASK) + MAX_BLOCKS_IN_GROUP); // Make sure there's no conflict with existing random block IDs final Block b = new Block(getCurrentValue()); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java index c1d83736461ab7..26916bd602f923 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java @@ -203,10 +203,9 @@ public void testBlockGroupIdThreadSafety() throws Exception { Set allBlockIds = new HashSet<>(); for (List set : blockIds) { for (long id : set) { - if (allBlockIds.contains(id)) { + if (!allBlockIds.add(id)) { fail("Same block group id is generated!"); } - allBlockIds.add(id); } } } From 5de876ce05d5682f2b4ab7aa1ccb13e5fe69f58a Mon Sep 17 00:00:00 2001 From: zhangshuyan Date: Wed, 14 Jun 2023 16:50:36 +0800 Subject: [PATCH 3/3] add some comments. --- .../server/blockmanagement/TestSequentialBlockGroupId.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java index 26916bd602f923..8240cc48625eaa 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestSequentialBlockGroupId.java @@ -183,8 +183,10 @@ public void testTriggerBlockGroupIdCollision() throws IOException { */ @Test public void testBlockGroupIdThreadSafety() throws Exception { + // Each thread use a list to store its own block group IDs. List> blockIds = new ArrayList<>(); List threads = new ArrayList<>(); + for (int i = 0; i < 20; i++) { blockIds.add(new ArrayList<>()); threads.add(new Thread(() -> { @@ -200,6 +202,7 @@ public void testBlockGroupIdThreadSafety() throws Exception { for (Thread t : threads) { t.join(); } + // Check if there are duplicate IDs. Set allBlockIds = new HashSet<>(); for (List set : blockIds) { for (long id : set) {