chore: Create a new temp object when writing lease all the time. #2776
Conversation
The temp path is generated with a random UUID as prefix. Signed-off-by: Vaibhav <vrongmeal@gmail.com>
|
@greyscaled we might need a background job to clean up these objects? Not sure if a job exists right now. If there is, might need to modify it. |
…passed Signed-off-by: Vaibhav <vrongmeal@gmail.com>
| if expires_at - LEASE_DURATION + (LEASE_RENEW_INTERVAL / 2) > now { | ||
| // Don't acquire a new lease. Let the process use the existing | ||
| // lease. This should help in reducing the number of writes to | ||
| // the lease object. | ||
| return Ok(lease.generation); | ||
| } | ||
|
|
There was a problem hiding this comment.
I just want to clarify things:
- is there a way that the same catalog could be modified or accessed by two tasks/threads within the same process? My concern is that this lease could be (unintentionally) shared between two orthogonal processes.
- what is the case when the lease is (actually) renewed, and in what cases are leases released? Does this change any of those points?
There was a problem hiding this comment.
is there a way that the same catalog could be modified or accessed by two tasks/threads within the same process? My concern is that this lease could be (unintentionally) shared between two orthogonal processes.
So, I just took a better look and process_id refers to the service and not session. So, it's most definitely possible that two threads within the same process modify the same catalog! I should undo this change.
what is the case when the lease is (actually) renewed, and in what cases are leases released? Does this change any of those points?
Lease is renewed after the given interval. A lease is released once the mutation is over (or automatically if it expires).
There was a problem hiding this comment.
So, it's most definitely possible that two threads within the same process modify the same catalog! I should undo this change.
Maybe. If this is an existing flaw, and this change doesn't increase the likelihood of a collision, then it's probably safe to commit it. We could also change to something that isn't the process_id.
| if expires_at - LEASE_DURATION + (LEASE_RENEW_INTERVAL / 2) > now { | ||
| // Don't acquire a new lease. Let the process use the existing | ||
| // lease. This should help in reducing the number of writes to | ||
| // the lease object. | ||
| return Ok(lease.generation); | ||
| } | ||
|
|
There was a problem hiding this comment.
I just want to clarify things:
- is there a way that the same catalog could be modified or accessed by two tasks/threads within the same process? My concern is that this lease could be (unintentionally) shared between two orthogonal processes.
- what is the case when the lease is (actually) renewed, and in what cases are leases released? Does this change any of those points?
To me, it seems like they would be in sub-dirs of |
|
@vrongmeal I think we discussed closing this because the existing solution (retries) works, we have a longer term plan to solve this issue, and this has some larger in-process concerns. |
What's the concern? The code seems reasonable to me. |
@scsmithr does the lease retention seem fine to you? I had some doubts about retaining a lease by the same process as Sam mentioned, a process might try to modify the same catalog from two different threads. But seems like that problem exists from before. We can merge this in as it is and should be OK (after thinking about it for a bit) but want to know your opinion. |
Two concurrent operations on the same instance would both assume that they held the lock. |
The temp path is generated with a random UUID as suffix.