Ensure ZK server failure in the middle of lock creation is handled

[keith-turner]

Accumulo use ephemeral sequential nodes in ZK for locks. Need to ensure the lock code handles a ZK server failing in the middle of a call to create an ephemeral sequential node.

https://github.com/apache/curator/blob/apache-curator-4.0.1/curator-framework/src/main/java/org/apache/curator/framework/api/CreateBuilderMain.java#L57

[DomGarguilo]

The attached link suggests the use of .withProtection() while creating the ephemeral sequential nodes. From what I can tell, this is an apache curator function which is not used in the creation of ephemeral sequential nodes which happens in ZooReaderWriter.java. I'm not sure of an alternative or a solution at this point. I know this is an old issue so I just wanted to share my initial thoughts/findings in case someone else has some insight into this.

[ctubbsii]

@DomGarguilo I think the idea was to see how Curator solves the issue, to see if we can adopt a similar strategy in the relevant code in Accumulo. We have previously attempted (a very long time ago) to use Curator directly, but it was a much bigger effort that was ultimately abandoned.

[EdColeman]

This "type" of issue was also discussed in #1086.

[DomGarguilo]

It seems that something like the .withProtection() in curator could be implemented to help with this issue. From what I understand, .withProtection() prefixes the name of the node being created with a GUID/UUID which, if creation fails, is searched for on the parent path and returned if found. In accumulo if node creation fails, depending on why it fails, the creation is either retried or an exception is thrown. My thinking is that during this retry, before an attempt is made to create a new node, the name of the initial node prefixed with the UUID is searched for and returned if found. At the moment I'm still looking into how this might be implemented and determining if this a viable solution.

[DomGarguilo]

To add this functionality it seems either a boolean value could be accepted by ZooReaderWriter.putEphemeralSequential() to denote whether "protection" would be used during creation, or an additional method could be created, something like putProtectedEphemeralSequential() to achieve the same effect. I think it would depend on whether any other variant of node may need to be created with this sort of protection as well as other implications of creating a new method or adding a new input value to the existing method. Still trying to work out the second piece as well: how search/recovery of a node from its UUID-prefixed path might work and where that logic should be implemented.

[EdColeman]

Curator is a more general framework and is probably why they allow for the additional flexibility, but where we are creating / using locks, I believe that we would always want to have the stronger guarantee. In addition to the curator code, the zookeeper recipe documentation also lays out how this can be achieved. (https://zookeeper.apache.org/doc/r3.5.8/recipes.html#sc_recipes_Locks)

For Accumulo, I believe the pattern we currently expect for lock names is lock-[sequential id] - if this were modified to be lock-[uuid]-[sequential id] then the basic recipe would work.

This is roughly the same sequence outlined by the zookeeper recipe, modified with our current naming convention. This assumes we only create one "lock type" in a path - so for example a table lock would be something like /accumulo/[instance-id]/table_locks/[table_id]/ any node named lock-* under that path is only for locking that table (a unique id / path) - other locks would use a different path / root.

With this convention:

Create a sequential, ephemeral lock node: lock-[uuid] and allow zookeeper to assign the sequential id. The generated name would be of the form lock-[uuid]-######## with the # assigned by zookeeper.
Call getChildren (no watch) on the path where the lock node was created to get the node names.
Filter on lock-*
Sort using only the sequential id, and find the node with the lowest sequential number.
The node with the lowest sequential id "has the lock"

• If the uuid of the node with the lowest sequential id is the uuid that the process assigned, then that process has the lock.
• If the lowest sequential id has a different uuid, then someone else has the lock.

I had started implementing this approach before 1.10 was released, but we determined that it would be more that a "bug-fix" and would not included in that release. So, I put it aside to work other issues. I'll need to check to see what additional modifications are required for 2.1. There were other related issues regarding using the session id that I believe that the can also be removed with using this approach for locks.

[ctubbsii]

@EdColeman Wouldn't including a UUID mean that we'd get a bunch of nodes colliding with the "lowest" sequential ID, because they are all unique? Something like: lock-uuid1-1, lock-uuid2-1, lock-uuid3-1, etc.? Without the UUID, we get things that look like: lock-1, lock-2, lock-3, etc. Isn't that what we want? It seems to me that the UUID would be more useful in the value, so a node that believes itself to have the lowest lock can verify that its UUID matches the one seen, but it wouldn't affect the sequence.

[EdColeman]

@ctubbsii the sequence number is based on the parent node, not the node name. From the (zookeeper programming guide)

When creating a znode you can also request that ZooKeeper append a monotonically increasing counter to the end of path. This counter is unique to the parent znode.

I'm having env issues, but I'll post an example when I get those sorted.

[EdColeman]

The advantage of using the uuid in the name vs. putting it in as a value is that the node names and sequence can be determined by calling getChildren() and then parsing the name. If the uuid was only in the value, then you would need to call getChildren to get the node names, and then each client would need to call getData to read the uuid on at least the node with the lowest sequence id. If a client needed to know its place in the sequence, that client would need to keep calling getData until if found its uuid (if existed at all).

To confirm that the sequence id is based on the parent node I manually generated some nodes. The parent path is /lock_test/myLocks/ and the sequential, ephemeral nodes are named lock-9999-, lock-1234-, lock-9876- (and in this order) - the 9999 node should have the lowest sequential id. Looking in zookeeper, the nodes created are:

[zk: localhost:2181(CONNECTED) 5] ls /lock_test/myLocks [lock-1234-0000000001, lock-9876-0000000002, lock-9999-0000000000]
The nodes were created using:

` zk.create(
root + "/myLocks/lock-9999-",
null,
ZooDefs.Ids.OPEN_ACL_UNSAFE,
CreateMode.EPHEMERAL_SEQUENTIAL);

zk.create(
    root + "/myLocks/lock-1234-",
    null,
    ZooDefs.Ids.OPEN_ACL_UNSAFE,
    CreateMode.EPHEMERAL_SEQUENTIAL);

zk.create(
    root + "/myLocks/lock-9876-",
    null,
    ZooDefs.Ids.OPEN_ACL_UNSAFE,
    CreateMode.EPHEMERAL_SEQUENTIAL);

`
After the test exits, but with the zookeeper instance still running - the sequential ids continue to increase, but the order remains as expected. On the second run, the created nodes are:

[zk: localhost:2181(CONNECTED) 7] ls /lock_test/myLocks [lock-1234-0000000004, lock-9876-0000000005, lock-9999-0000000003]

[ctubbsii]

@EdColeman That all makes sense. I didn't realize the sequence was based on the parent node. Thanks for the explanation.

[DomGarguilo]

I had started implementing this approach before 1.10 was released, but we determined that it would be more that a "bug-fix" and would not included in that release. So, I put it aside to work other issues.

@EdColeman, how far did you get with the implementation? If you are not currently working on this, I could give it a shot. Do you have any existing code or suggestions regarding where/how the various parts of this fix could be implemented?

[DomGarguilo]

My current thinking is to append the UUID to the path passed to create() within ZooReaderWriter.putEphemeralSequential which should achieve the format discussed earlier: lock-[uuid]-########. I'm not too sure where the best place to implement the portion which recovers the created node via getChildren() when this error occurs or how exactly to detect when this error occurs.

[DomGarguilo]

It seems there could be two ways this issue could be handled. In the zookeeper recipes which was linked earlier, it does not explicitly say what should happen to a node once its path is recovered. It seems like the path to the recovered node could either be returned, or the node itself could be deleted and node creation restarted. In curator, it seems that when a node in this situation is found, it is scheduled to be deleted and an error is thrown (heres the code). I'm not sure which would work best however if the node is deleted with an exception thrown, this might work well within the retry loop that is used to create nodes in accumulo.