I am not sure if this correct. Can someone confirm what would go here?

This looks good. But you should add a blank line between the current lines 2 and 3. :)

Thanks. Done.

I need help with this - Does this make the tests skip if the environment doesn't have a Zookeeper extension installed?

Probably needs to be a class level annotation, not a generic comment in the file

This was in wrong place by mistake. Moved.

Can you add a modifier? I believe this version of Symfony supports it.

Made it private.

@param Key $key, but as it doesn't add anything, it can be removed

Replaced with@inheritdoc instead of removing it.

param and return annotations can be removed

Replaced with@inheritdoc instead of removing it.

please import the class

It is already imported. Removed the fully qualified path.

Probably needs to be a class level annotation, not a generic comment in the file

perhaps a small while loop and check every 10ms is sufficient? Should probably try to avoid long sleeps.

Have added a while loop of10 ms each but, still keeping the max sleep time to1 second

Does everything really have to be in a try here?

Wrapping the logic to get state in this as well in case it throws an exception.

"Since version 0.3.0, this method emits ZookeeperException and it's derivatives."

For__construct:"This method emits PHP error/warning when parameters count or types are wrong or could not init instance." The test should fail when this occurs, not be skipped, otherwise we'll never know if we made a mistake somewhere or if the connection can't be made. However, both cases are a legit reason to fail a test.

ForgetState:"This method emits PHP error/warning when it fails to get state of zookeeper connection. " I would very much like to know if this fails, because it might be a setup issue, I rather not miss this if something broke.

To me it sounds like the whole try/catch should be removed and themarkTestAsSkipped for the connection underneath the while loop, should trigger a failure instead. If there would be a setup error in travis, we would never be notified of this as everything will always be marked as skipped. This means the test suite will probably just keep reporting that everything is okay.

I agree with you on the first 2 points and have removed thetry and catch blocks.

However, I feel like connection errors should not be raised as failures because environments can be flaky for many reasons and we should probably have some other way to be monitoring those. I think that's what@jderusse also wanted to do with Redis, Memcached and other stores.

Having said that, how can I install zookeeper extension and run zookeeper server on the Travis?

I've got experience with php extensions and travis, so I can't help you on that. The result I'm aiming for, is that we can distinguish connection failures (faulty travis setup), from errors, so we know when the tests fail. Environments shouldn't be flaky, because it shouldn't be flaky in production either. If environmentsare flaky, that should be fixed asap.

Please use$this->expectException(LockConflictedException::class) right before it's supposed to be thrown. Right now ifgetStore() throws it, this test would pass, while we probably expect it on the secondsave($key)

This should be fine as is. IfgetStore throws an then it would fail saying thatFailed asserting that exception of type "WhateverException" matches expected exception "\Symfony\Component\Lock\Exception\LockConflictedException".getStore will never throw aLockConflictedException.

"getStore will never throw a LockConflictedException", not in this pull request. Tests should be robust, thus if someone throws an exception in another PR at an earlier point and this test does not fail, the test is wrong.

Removed this test as per@jderusse 's suggestion that I shouldn't be modifying the behavior from the parent class for this.

base64 encode on random bytes sounds like a better idea here.

Right. Fixed.

Symfony does not "import" classes without namespace. Use directly\Throwable when needed

Do you mean they don't get autoloaded without namespace? These should be in global namespace and it works fine when I test locally. I don't know if it's a standard but I prefer declaring them at top so I don't have to add\ for every usage. If I am missing something can you please elaborate on this? Thanks!

In PHP you have two options when working with namespace:

1. importing the namespace (what you have done)
2. using it directly (ex: new \Foo\Bar)

In Symfony, we never import global class in the global namespace (like Throwable, DateTime, SplFileObject, Exception ...)

So you have to update your code to remove theuse Throwable, then use directly\Throwable

Thanks

Thanks@lyrixx ! I will update the code.

"Since version 0.3.0, this method emits ZookeeperException and it's derivatives."

For__construct:"This method emits PHP error/warning when parameters count or types are wrong or could not init instance." The test should fail when this occurs, not be skipped, otherwise we'll never know if we made a mistake somewhere or if the connection can't be made. However, both cases are a legit reason to fail a test.

ForgetState:"This method emits PHP error/warning when it fails to get state of zookeeper connection. " I would very much like to know if this fails, because it might be a setup issue, I rather not miss this if something broke.

To me it sounds like the whole try/catch should be removed and themarkTestAsSkipped for the connection underneath the while loop, should trigger a failure instead. If there would be a setup error in travis, we would never be notified of this as everything will always be marked as skipped. This means the test suite will probably just keep reporting that everything is okay.

Looks like a duplicate ofconst LOCK_NODE

It's not a duplicate, but rather a prefix for every lock node. For example:const LOCK_NODE = PHP_LOCK_NODE, while prefix can beORDER, so for anORDER ID 1234, the lock would be created as/PHP_LOCK_NODE/ORDER_1234 I see that theprefix is not really required and the client can pass this in as theresource itself. So removed it. I also removed theconst LOCK_NODE as we can let the client decide how they want to organize the nodes.

no need to use multiline declaration

Fixed.

I don't get this comment

Bad copy paste. Removed.

Who is in charge of cleaning all this intermediate path?

Wouldn't it be easier to provide a node$node without/?

It would be easier, but I also wanted to provide the functionality of having intermediate nodes if anyone wanted to organize those. Ideally it would be preferred to have just/node for the sake of simplicity, but I can specify this in the documentation as a recommendation if that works for you.

IMHO the client shouldn't have to know how stores works internally, and doesn't need to manipulate it. And I'm not sure that organizing "ephemeral nodes" worth it.
Moreover, the service asking a Lock may not (shouldn't?) be aware of the store used. It may not be aware using keys with slashes have a special meaning.

The issue with this feature are:

• it add a lot of complexity to the method
• it requires to create intermediate nodes that you can't delete theme afterward. what if a jobs locks/foo/{id}/bar and iterate over billions ofids?

Do you have a use case in mind?

I was following the recipe from here:https://zookeeper.apache.org/doc/r3.1.2/recipes.html#sc_recipes_Locks

But that protocol was recommendingephermeal + sequence nodes. Since in our case we don't use sequence nodes, I think it is fine to not support multi-level nodes.

Changed the implementation to support single ephemeral node. In this case, I am validating the input during lock creation. Also added a test case for this.

while (count($parts)>1)

Fixed.

SHouldn't it be aLockConflictException?

If you see the error handler, if another node was created due to a race condition, it would throw aLockConflictedException. If it reaches here, it means it failed to acquire the lock for some other reason.

note used

Removed.

Add a comment saying why there is nothing to do

Added.

what if the node is removed (by a conurrent request) just before this line?

IMHO you can remove thethis->zookeeper->exists and wrap thisthis->zookeeper->get in a try/catch block

The lock can only be removed by the original owner and no other concurrent request can delete it unless someone does it via command line or write a script to delete the nodes.

The originaltestSaveTwice asserts that the store is able to save the same Key twice. This is the expected behavior of a Store.

If the test does not pass, I suggest to fix the implementation of the store instead of changing the behavior of the test...

You are right, thanks for highlighting that. Removed this function from here.

How does Zookeeper handle integrity when using a cluster of servers?
Are you sure that creating an existing node would raise an exception even if 2 concurrent requests ask the creation at exactly the same time on 2 distinguished servers?

Great question - Zookeeper always has only 1 leader and accepts writes through that leader and uses quorum, so even if it due to some race condition 2 process try to create a node, the second one will raise a warning and eventuallyLockConflictedException will be thrown.