110y/yo to lgruen/yo. Could you reconfirm it?
I'll take a look at the CircleCI integration tomorrow if that's okay. Is there anything I should look out for? Is all that's needed this set of environment variables? (https://github.com/cloudspannerecosystem/yo/blob/master/.circleci/config.yml#L48)
Is all that's needed this set of environment variables?
If you could send another description snippet for the website, that would be great!
- Would you be able to give some background on why you're using this approach in your production app? Better still, if you could document some use cases in the README, that would be great.
- Why did you choose Spanner for this over other databases?
@skuruppu No problem at all.
Most of our teams employ some sort of distributed processing in our services, and distributed locking is quite a common technique in this area. We use some of the more well known ones such as Zookeeper and Consul. These systems are quite difficult to maintain by themselves though, so any option that are easier when it comes to maintenance are welcome to us. Redis is also common in our backends and I even have a Redis-based distributed locker lib https://github.com/flowerinthenight/kettle that we use as well. But the Redis version is not really that reliable as described here. This would lead me to:
We use Spanner heavily as one of our main databases, among others. So if I could do distributed locking using Spanner itself, I don't have to maintain another Zookeeper just for locking. I think TrueTime is crucial here and may even be more reliable than the Redis version, time will tell. So far, our Zookeepers are the most reliable but I think that this library can be as reliable as well, which it is, so far.
- I still need to figure out the official process we should use to move the repo. I'll get back to you with details next week.
- We'll need to add some tests. I'm not sure if it's important to run the tests against the production backend or whether it's possible to run it against the Cloud Spanner Emulator. Is this something you've looked into?
- We'll need to do a code review. We can do this before or after the move. @olavloite would you be able to take a quick look at the code and provide any feedback you have about the approach?
- (More of a general question) If I understand it correctly, the current implementation allows multiple records for the same lock to exist in the table, and uses a process of adding new records when a new lock is acquired and deletes old records that are no longer valid. Would it also be possible to change this to an implementation that only allows one record per lock (enforced with a unique index or even through the primary key of the lock table)? I think that would make the implementation slightly simpler and also more robust, as Cloud Spanner could then be used to not only supply the lock timestamp but also the uniqueness of the lock. It would also make it possible to use mutations instead of DML, which again allows for the use of InsertOrUpdate mutations. That could simplify the current implementation that currently has specific handling for the first insertion, and the update method that executes separate UPDATE and DELETE statements. Or am I missing something here that makes such an implementation impossible / unfeasible?
- Could you look into breaking up the current
Runfunction into a couple of separate functions? The current nesting of a combination of functions and loops is quite deep and that makes it difficult to grasp exactly what is going on there.
@skuruppu For #2, yes I was looking into the possible use of the emulator for tests but haven't got the time to try and implement it yet.
@olavloite Thanks very much. One record equals a single lock. Since the name column is the primary key, it should be unique. Throughout the duration of the lock lease, that single record (the one that's grabbed during initial) will be used, with the heartbeat and timestamp columns being updated periodically. During the initial iteration, it is important to use INSERT, and not InsertOrUpdate to make sure that only one client/process will succeed and all others fail. The same story with the succeeding attempts to grab the lock. It needs to use INSERT, not InsertOrUpdate. The cleanup part is actually not necessary. It's just there to keep the table contents small and will be done by the current lock owner. If removed, and, say, we have a lock duration of 1sec, a new record will be added to the table every second. This bit probably needs a better implementation.
For the 2nd one, yes you're right. It could do with a bit of breaking down. I'll make some improvements hopefully sooner when I can find some spare time. I just have to make sure that changes will not affect our current production.
Thanks @flowerinthenight, we would like to have some tests before we publish it in the repo.
For the publishing process, I think we can create a repo in the ecosystem then do a repo move. I will have to go through some internal processes to create the repo. Note that you and anyone else that has committed to the repo will need to sign a Google CLA (https://cla.developers.google.com/about/google-individual). Would this be ok with you and the other contributors?