[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/303#discussion_r251851979
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -610,18 +610,18 @@ private String protectedPathInForeground(String
adjustedPath, byte[] data, List<
{
ThreadUtils.checkInterrupted(e);
if ( ( e instanceof KeeperException.ConnectionLossException ||
-!( e instanceof KeeperException )) && protectedId != null )
+!( e instanceof KeeperException )) &&
protectedMode.doProtected() )
--- End diff --
Note: this is a subtle change. The previous version checked `protectedId
!= null` and I don't remember why.
---
[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/303#discussion_r251685066
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -48,19 +50,21 @@
public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2,
BackgroundOperation, ErrorListenerPathAndBytesable
{
+private final Logger log = LoggerFactory.getLogger(getClass());
private final CuratorFrameworkImpl client;
private CreateMode createMode;
private Backgrounding backgrounding;
private boolean createParentsIfNeeded;
private boolean createParentsAsContainers;
-private boolean doProtected;
private boolean compress;
private boolean setDataIfExists;
private int setDataIfExistsVersion = -1;
-private String protectedId;
private ACLing acling;
private Stat storingStat;
private long ttl;
+private boolean doProtected;
+private String protectedId;
+private long initialSessionId;
--- End diff --
protectedEphemeralSessionID? And then maybe move the initialisation check
so that it only occurs if Curator is actually creating a protected ephemeral
node, rather than always initialising on the first forPath() call?
---
[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user Randgalt commented on a diff in the pull request:
https://github.com/apache/curator/pull/303#discussion_r251684425
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -48,19 +50,21 @@
public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2,
BackgroundOperation, ErrorListenerPathAndBytesable
{
+private final Logger log = LoggerFactory.getLogger(getClass());
private final CuratorFrameworkImpl client;
private CreateMode createMode;
private Backgrounding backgrounding;
private boolean createParentsIfNeeded;
private boolean createParentsAsContainers;
-private boolean doProtected;
private boolean compress;
private boolean setDataIfExists;
private int setDataIfExistsVersion = -1;
-private String protectedId;
private ACLing acling;
private Stat storingStat;
private long ttl;
+private boolean doProtected;
+private String protectedId;
+private long initialSessionId;
--- End diff --
Sure - can you suggest a name?
---
[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user cammckenzie commented on a diff in the pull request:
https://github.com/apache/curator/pull/303#discussion_r251672915
--- Diff:
curator-framework/src/main/java/org/apache/curator/framework/imps/CreateBuilderImpl.java
---
@@ -48,19 +50,21 @@
public class CreateBuilderImpl implements CreateBuilder, CreateBuilder2,
BackgroundOperation, ErrorListenerPathAndBytesable
{
+private final Logger log = LoggerFactory.getLogger(getClass());
private final CuratorFrameworkImpl client;
private CreateMode createMode;
private Backgrounding backgrounding;
private boolean createParentsIfNeeded;
private boolean createParentsAsContainers;
-private boolean doProtected;
private boolean compress;
private boolean setDataIfExists;
private int setDataIfExistsVersion = -1;
-private String protectedId;
private ACLing acling;
private Stat storingStat;
private long ttl;
+private boolean doProtected;
+private String protectedId;
+private long initialSessionId;
--- End diff --
I think that maybe the initialSessionId should have a different name as
it's a bit misleading. It's not really the initialSessionId as it gets updated
during the edge case for handling protected ephemeral nodes. Really, it's the
session ID at the time of creation for a protected ephemeral node isn't it?
Perhaps it should be named accordingly and only initialised for the case where
it will be needed?
---
[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user Randgalt commented on a diff in the pull request: https://github.com/apache/curator/pull/303#discussion_r251599266 --- Diff: curator-client/src/main/java/org/apache/curator/utils/InjectSessionExpiration.java --- @@ -94,7 +89,7 @@ public static void injectSessionExpiration(ZooKeeper zooKeeper) Object eventThread = eventThreadField.get(clientCnxn); queueEventMethod.invoke(eventThread, event); queueEventOfDeathMethod.invoke(eventThread); --- End diff -- TBH I just wanted to make the minimal change needed to get this to work. It's been this way a very long time. ---
[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
Github user shayshim commented on a diff in the pull request: https://github.com/apache/curator/pull/303#discussion_r251593572 --- Diff: curator-client/src/main/java/org/apache/curator/utils/InjectSessionExpiration.java --- @@ -94,7 +89,7 @@ public static void injectSessionExpiration(ZooKeeper zooKeeper) Object eventThread = eventThreadField.get(clientCnxn); queueEventMethod.invoke(eventThread, event); queueEventOfDeathMethod.invoke(eventThread); --- End diff -- Why it is important to queue event of death immediately? It will be done for us soon by this flow: ConnectionState.process -> handleExpiredSession -> ... -> ClientCnxn.disconnect ->... - because we just queued this WatchedEvent. Same thing for wakeupCnxnMethod.invoke. ---
[GitHub] curator pull request #303: [CURATOR-498] Protected Mode creation can mistake...
GitHub user Randgalt opened a pull request: https://github.com/apache/curator/pull/303 [CURATOR-498] Protected Mode creation can mistake closing session's node causing problems for many recipes such as LeaderLatch Kudos to user Shay Shimony for his tireless and excellent work tracking this down. There are two problems addressed here: 1) Protected create mode can potentially find a ZNode that is about to be deleted due to an expired session. CreateBuilderImpl now keeps track of the session ID when the create is initiated. If after a connection loss the session ID has changed, any found protected node is ignored as it will soon be deleted. 2) For ZooKeeper 3.4.x the simulated (via reflection) InjectSessionExpiration was incorrectly setting the connection state to closed which caused the session expiration to be ignored. You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/curator CURATOR-498 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/curator/pull/303.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #303 commit ea505f54291dc548aca947503630960cd10225d0 Author: randgalt Date: 2019-01-28T19:23:15Z CURATOR-498 Kudos to user Shay Shimony for his tireless and excellent work tracking this down. There are two problems addressed here: 1) Protected create mode can potentially find a ZNode that is about to be deleted due to an expired session. CreateBuilderImpl now keeps track of the session ID when the create is initiated. If after a connection loss the session ID has changed, any found protected node is ignored as it will soon be deleted. 2) For ZooKeeper 3.4.x the simulated (via reflection) InjectSessionExpiration was incorrectly setting the connection state to closed which caused the session expiration to be ignored. ---
