xline-kv / Xline

A geo-distributed KV store for metadata management
https://xline.cloud
Apache License 2.0
562 stars 70 forks source link

Fix/issue 684 #820

Closed Phoenix500526 closed 4 days ago

Phoenix500526 commented 1 month ago

Please briefly answer these questions: Close the issue #664 and #684

mergify[bot] commented 1 month ago

@Phoenix500526 Convert your pr to draft since CI failed

mergify[bot] commented 1 month ago

@Phoenix500526 You've modified the workflows. Please don't forget to update the .mergify.yml.

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 70.04831% with 62 lines in your changes missing coverage. Please review.

Project coverage is 75.57%. Comparing base (e35b35a) to head (09f1df9). Report is 132 commits behind head on master.

Files Patch % Lines
crates/xline-client/src/clients/lock.rs 73.97% 38 Missing and 13 partials :warning:
crates/xlinectl/src/command/lock.rs 0.00% 6 Missing :warning:
crates/xlinectl/src/command/lease/keep_alive.rs 0.00% 5 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #820 +/- ## ========================================== + Coverage 75.55% 75.57% +0.02% ========================================== Files 180 186 +6 Lines 26938 27712 +774 Branches 26938 27712 +774 ========================================== + Hits 20353 20944 +591 - Misses 5366 5472 +106 - Partials 1219 1296 +77 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mergify[bot] commented 1 month ago

@Phoenix500526 Convert your pr to draft since CI failed

mergify[bot] commented 1 month ago

@Phoenix500526 Your PR is in conflict and cannot be merged.

mergify[bot] commented 1 month ago

@Phoenix500526 Your PR is in conflict and cannot be merged.

Phoenix500526 commented 1 month ago

Mutex and its guard may be not the best way to abstract. If there's any error happens, such as lease renew failure, lock free is a good point to report error. In the mutex guard abstraction, lock free is in the drop function, which is not possible to handle for the caller.

But if you don't provide an RAII implementation for the lock, users may be bothered by the fact that they may forget to free a lock.

rogercloud commented 1 month ago

Mutex and its guard may be not the best way to abstract. If there's any error happens, such as lease renew failure, lock free is a good point to report error. In the mutex guard abstraction, lock free is in the drop function, which is not possible to handle for the caller.

But if you don't provide an RAII implementation for the lock, users may be bothered by the fact that they may forget to free a lock.

No, they won't if you put operations dealing with the locked data in a closure and release the lock while leaving the closure. For example providing the following code:

let return_value = a_xutex.map_lock( |xutex_guard| {
/// dealing with the guard
});

The return_value can tell if there's any lock related issue during the closure.

Additionally if you follow this way, the life time bound of the guard is not necessary as there's only one way to get the guard and drop it.

mergify[bot] commented 3 weeks ago

@Phoenix500526 Convert your pr to draft since CI failed

mergify[bot] commented 2 weeks ago

@Phoenix500526 Your PR is in conflict and cannot be merged.

mergify[bot] commented 1 week ago

@Phoenix500526 Convert your pr to draft since CI failed

bsbds commented 1 week ago

We could guarantee the lock safety by coupling the lock key to every update send to Xline, and the Xline server must verify the validity of the key. Please refer to https://jepsen.io/analyses/etcd-3.4.3

On the client side, we could associate KV operation methods to the lock guard to prevent user from using the lock for other purpose.

Phoenix500526 commented 1 week ago

@Mergifyio rebase

mergify[bot] commented 1 week ago

rebase

✅ Branch has been successfully rebased