Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CURATOR-493] Fix test case with testDeleteChildrenConcurrently #291

Merged
merged 1 commit into from
Dec 9, 2018
Merged

[CURATOR-493] Fix test case with testDeleteChildrenConcurrently #291

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 54 additions & 16 deletions curator-framework/src/test/java/org/apache/curator/framework/imps/TestFrameworkEdges.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,13 @@
import org.apache.zookeeper.WatchedEvent;
import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.data.Stat;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testng.Assert;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
import java.util.List;
import java.util.Random;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.BlockingQueue;
import java.util.concurrent.CountDownLatch;
Expand All @@ -59,10 +62,13 @@

public class TestFrameworkEdges extends BaseClassForTests
{

private final Logger log = LoggerFactory.getLogger(getClass());
private final Timing2 timing = new Timing2();

@BeforeClass
public static void setUpClass() {
public static void setUpClass()
{
System.setProperty("zookeeper.extendedTypesEnabled", "true");
}

Expand Down Expand Up @@ -668,12 +674,13 @@ public void testDeleteChildrenConcurrently() throws Exception
client.create().creatingParentsIfNeeded().forPath("/parent/child" + i);
}

final CountDownLatch countDownLatch = new CountDownLatch(1);
final CountDownLatch latch = new CountDownLatch(1);
new Thread(new Runnable()
{
@Override
public void run()
{
long start = System.currentTimeMillis();
try
{
client.delete().deletingChildrenIfNeeded().forPath("/parent");
Expand All @@ -686,35 +693,66 @@ public void run()
}
else
{
Assert.fail("unknown exception", e);
Assert.fail("unexpected exception", e);
}
}
finally
{
countDownLatch.countDown();
log.info("client has deleted children, it costs: {}ms", System.currentTimeMillis() - start);
latch.countDown();
}
}
}).start();

Thread.sleep(20L);
try
{
client2.delete().forPath("/parent/child" + (childCount / 2));
}
catch ( Exception e )
boolean threadDeleted = false;
boolean client2Deleted = false;
Random random = new Random();
for ( int i = 0; i < childCount; i++ )
{
if ( e instanceof KeeperException.NoNodeException )
String child = "/parent/child" + random.nextInt(childCount);
try
{
Assert.fail("client2 delete failed, shouldn't throw NoNodeException", e);
if ( !threadDeleted )
{
Stat stat = client2.checkExists().forPath(child);
if ( stat == null )
{
// the thread client has begin deleted the children
threadDeleted = true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of this? Once it is set, checkExists() is never called again. That seems incorrect. I removed this and the test still passes. Can you explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that judge the new thread is running at the loop that is deleting children:
ZKPaths.java , 341 line

        for ( String child : children )
        {
            String fullPath = makePath(path, child);
            deleteChildren(zookeeper, fullPath, true);
        }

and then, the main thread is try to delete some child that is still not deleted by the new thread. If the main thread delete child successfully, the new thread should not throw NoNodeException, but the old version will throw NoNodeException and cause the remaining children are not deleted, that's the bug situation.

So, it's unnecessary to checkExists() after that threadDeleted is true. I just want the main thread delete a child successfully, when the new thread is deleting children.

log.info("client has deleted the child {}", child);
}
}
else
{
try
{
client2.delete().forPath(child);
client2Deleted = true;
log.info("client2 deleted the child {} successfully", child);
break;
}
catch ( Exception e )
{
if ( e instanceof KeeperException.NoNodeException )
{
// ignore, because it's deleted by the thread client
}
else
{
Assert.fail("unexpected exception", e);
}
}
}
}
else
catch ( Exception e )
{
Assert.fail("unknown exception", e);
Assert.fail("unexpected exception", e);
}
}

Assert.assertTrue(countDownLatch.await(10, TimeUnit.SECONDS));

// The case run successfully, if client2 deleted a child successfully and the client deleted children successfully
Assert.assertTrue(client2Deleted);
Assert.assertTrue(timing.awaitLatch(latch));
Assert.assertNull(client2.checkExists().forPath("/parent"));
}
finally
Expand Down