xline-kv / Xline

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

feat: add exec command args for xlinectl lock command #846

Closed bhavik-goplani closed 3 days ago

bhavik-goplani commented 2 weeks ago

Please briefly answer these questions:

Additional comments: I'm not sure how to test the exec command args. I thought of two solutions:

  1. Extend LockRequest or Test Case Representation: Add a way to represent the exec commands that should be executed after the lock is acquired. This could be a new field in the LockRequest struct or a separate structure linked to the test case.
  2. Modify the Test Case: Adjust the test case in macros.rs to include the exec commands as part of the expected outcome.
codecov[bot] commented 2 weeks ago

Codecov Report

Attention: Patch coverage is 36.36364% with 14 lines in your changes missing coverage. Please review.

Project coverage is 75.61%. Comparing base (e35b35a) to head (038bca5). Report is 108 commits behind head on master.

Files Patch % Lines
crates/xlinectl/src/command/lock.rs 36.36% 14 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #846 +/- ## ========================================== + Coverage 75.55% 75.61% +0.05% ========================================== Files 180 187 +7 Lines 26938 27685 +747 Branches 26938 27685 +747 ========================================== + Hits 20353 20933 +580 - Misses 5366 5465 +99 - Partials 1219 1287 +68 ```

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

CrystalAnalyst commented 4 days ago

Sorry for the late response. I really appreciate your interest about Xline and the contribution you've made. I've reviewd your code logic, it's natural and clear. However, recently we did a major change about this xlinectl lock command, you can refer to the #820. In essence, the lock usage is changed from : client.lock_client().lock(req).await?; to : let mut xutex = Xutex::new().await?; let xutex_guard = xutex.lock_unsafe().await? So you may update your repo and make corresponding change, especially the : pub(crate) async fn execute(client: &mut Client, matches: &ArgMatches) -> Result<()>. And for the test, we have a validation test in the: Xline/scripts/validation_test.sh, you can add your test case here. If you have any questions, feel free to ask.

bhavik-goplani commented 3 days ago

@CrystalAnalyst Thanks for the feedback. I'll close this pull request and create a new PR after pulling the recent changes.