-
Notifications
You must be signed in to change notification settings - Fork 487
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.
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
etcd3 Locker: First pass #202
Conversation
Thanks a lot for this work @chen-anders! Just a quick note that our maintainer is on holidays so it might take a bit before you'll have this thoroughly reviewed. |
f7c282e
to
15f624c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for this amazing PR. Apologies for my delayed response but as @kvz said I have been quite busy the last weeks. That being said I have a few questions about this but I am eager to see this landing in tusd.
6ba04a0
to
c25194e
Compare
Thanks for the updates and comments. I will have a look at them in the next few days. Would you mind looking into the test suite failures (https://travis-ci.org/tus/tusd/builds/431249928) in the meantime? |
- Introduce LockerOptions to allow a custom session TTL (default: 60s); etcd3Locker can be initialized with NewWithLockerOptions if one wants granular control over TTL and prefix used for etcd3 keys - Keep NewWithPrefix backwards compatible by calling NewWithLockerOptions
fc180b2
to
a1b03a1
Compare
@Acconut - Tests are now passing. |
@Acconut - this is ready for another look-over. |
Thank you very much for this amazing PR and especially dealing with my questions, we appreciate this a lot 👍 Only one question: Is there an update regarding #202 (comment)? |
The coroutine leak here was a red herring since we ended up finding an issue where writing straight to the filesystem (instead of using a mounted volume) in Docker causing heavy page cache use (when using the S3 plugin). In our Kubernetes clusters, this registered as continuously increasing memory usage. The simplification made some time ago did make it so unnecessary leases were no longer being opened, but I don't think that was causing any major memory usage. As part of our investigation, we SSH'd into the container and did a |
I am glad to hear that there was no goroutine leak, thank you for the detailed answer. |
Resolves: #157
This is a first pass at an etcd3 locker that uses the built-in locks (from the
etcd/clientv3/concurrency
package). We create sessions with valid leases to lock an upload. Sessions are created with a renewing 60s lease (but kept alive with KeepAlive as long as the process is still alive). These sessions are reused to unlock uploads when the upload completes. Once an upload has been unlocked, we close the session.This code has been tested locally with an install of
etcd3
(3.1, 3.2, 3.3) with the use of thego-etcd-harness
.We're also running this locker with some small scale traffic to our tusd server, in which we haven't seen issues so far.
Example Usage (pseudocode):