Skip to content

CURATOR-228 - Modified the background callback to explicitly handle t… #93

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

Merged
asfgit merged 2 commits into from
Aug 31, 2015
Merged

CURATOR-228 - Modified the background callback to explicitly handle t… #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ecf67df
CURATOR-228 - Modified the background callback to explicitly handle t…
cammckenzie Jul 23, 2015
cff86ea
CURATOR-228 - Remove public access from the isAuthFailure method.
cammckenzie Jul 23, 2015
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
16 changes: 16 additions & 0 deletions ...pes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentEphemeralNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.apache.curator.framework.recipes.nodes;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;

import org.apache.curator.framework.CuratorFramework;
Expand All @@ -41,6 +42,7 @@
import java.util.Arrays;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;

import org.apache.curator.utils.PathUtils;
Expand All @@ -65,6 +67,7 @@ public class PersistentEphemeralNode implements Closeable
private final Mode mode;
private final AtomicReference<byte[]> data = new AtomicReference<byte[]>();
private final AtomicReference<State> state = new AtomicReference<State>(State.LATENT);
private final AtomicBoolean authFailure = new AtomicBoolean(false);
private final BackgroundCallback backgroundCallback;
private final Watcher watcher = new Watcher()
{
Expand Down Expand Up @@ -233,8 +236,15 @@ else if ( event.getResultCode() == KeeperException.Code.OK.intValue() )
{
path = event.getName();
}
else if ( event.getResultCode() == KeeperException.Code.NOAUTH.intValue() )
{
log.warn("Client does not have authorisation to write ephemeral node at path {}", path);
Copy link
Copy Markdown
Member

@Randgalt Randgalt Jul 23, 2015

Choose a reason for hiding this comment

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

Sorry to be pedantic. But, isn't this going to be the same issue for InterProcessMutex, etc.? i.e. any ZK recipe that creates nodes - which is a lot of them. I think it's a mistake to have specific handling like this in a single recipe.

Copy link
Copy Markdown
Contributor Author

@cammckenzie cammckenzie Jul 23, 2015

Choose a reason for hiding this comment

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

Possibly, we probably need to do an audit. I had a look at the InterProcessMutex and I think that it's ok because the node creation happens in the foreground. The PersistentEphemeralNode is problematic because it's all done in the background.

Copy link
Copy Markdown
Member

@Randgalt Randgalt Jul 23, 2015

Choose a reason for hiding this comment

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

It would be better to just log the bad result code. Is there a reason that the check for NOAUTH is needed? Why not just log all non-expected result codes?

Copy link
Copy Markdown
Contributor Author

@cammckenzie cammckenzie Jul 23, 2015

Choose a reason for hiding this comment

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

Maybe we should just log an error and stop processing for any code that is not in RetryLoop.shouldRetry()? I was just trying to minimise the impact of the change as it was previously just handling the NodeExists and OK cases and retrying everything else.

authFailure.set(true);
return;
}
if ( path != null )
{
authFailure.set(false);
nodePath.set(path);
watchNode();

Expand Down Expand Up @@ -406,4 +416,10 @@ private boolean isActive()
{
return (state.get() == State.STARTED);
}

@VisibleForTesting
boolean isAuthFailure()
{
return authFailure.get();
}
}
39 changes: 39 additions & 0 deletions ...src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentEphemeralNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
import org.apache.zookeeper.CreateMode;
import org.apache.zookeeper.WatchedEvent;
import org.apache.zookeeper.Watcher;
import org.apache.zookeeper.ZooDefs;
import org.apache.zookeeper.Watcher.Event.EventType;
import org.apache.zookeeper.data.ACL;
import org.apache.zookeeper.data.Stat;
import org.testng.Assert;
import org.testng.annotations.AfterMethod;
Expand Down Expand Up @@ -572,6 +574,43 @@ public void testProtected() throws Exception
node.close();
}
}

@Test
public void testNoWritePermission() throws Exception
{
CuratorFrameworkFactory.Builder builder = CuratorFrameworkFactory.builder();
CuratorFramework client = builder
.connectString(server.getConnectString())
.authorization("digest", "me1:pass1".getBytes())
.retryPolicy(new RetryOneTime(1))
.build();
client.start();

ACL acl = new ACL(ZooDefs.Perms.WRITE, ZooDefs.Ids.AUTH_IDS);
List<ACL> aclList = Lists.newArrayList(acl);
client.create().withACL(aclList).forPath(DIR, new byte[0]);
client.close();

PersistentEphemeralNode node = null;
try {
//New client without authentication
client = newCurator();

node = new PersistentEphemeralNode(client, PersistentEphemeralNode.Mode.EPHEMERAL, PATH,
new byte[0]);
node.start();

node.waitForInitialCreate(timing.seconds(), TimeUnit.SECONDS);
assertNodeDoesNotExist(client, PATH);
assertTrue(node.isAuthFailure());
} finally {
if(node != null) {
node.close();
}

client.close();
}
}

private void assertNodeExists(CuratorFramework curator, String path) throws Exception
{
Expand Down