Skip to content

Unlock the LOCK files on drop #379

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.

Sign up for GitHub

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

Jump to bottom
Merged
ti-chi-bot merged 4 commits into from
May 20, 2025
Merged

Unlock the LOCK files on drop #379

ti-chi-bot merged 4 commits into from
May 20, 2025

Conversation

@v01dstar
Copy link
Member

@v01dstar v01dstar commented May 14, 2025 •
edited
Loading

We need to restart the Raft Engine within a test case to apply a configuration change. To do this, we must ensure the LOCK files are released upon drop, allowing the engine to be reopened within the same process.

Also update rustc version and fix clippy errors, since some dependencies require higher versions of Rust

@v01dstar
Unlock the LOCK files on drop, so we can open them again in the same …
6f912ef 
…process

Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
@ti-chi-bot ti-chi-bot bot added the dco-signoff: yes Indicates the PR's author has signed the dco. label May 14, 2025
@v01dstar v01dstar requested a review from LykxSassinator May 14, 2025 21:27
@ti-chi-bot ti-chi-bot bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 14, 2025
@v01dstar
Update rustc version
458c431 
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
@LykxSassinator
Copy link
Contributor

LykxSassinator commented May 16, 2025

/test

@ti-chi-bot
Copy link

ti-chi-bot bot commented May 16, 2025

@LykxSassinator: The /test command needs one or more targets.
The following commands are available to trigger required jobs:

/test rust-nightly

/test rust-stable

Use /test all to run the following jobs that were automatically triggered:

rust-nightly
Details

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@LykxSassinator
Copy link
Contributor

LykxSassinator commented May 16, 2025

/retest

1 similar comment
@v01dstar
Copy link
Member Author

v01dstar commented May 16, 2025

/retest

overvenus
overvenus reviewed May 16, 2025
View reviewed changes
Cargo.toml
authors = ["The TiKV Project Developers"]
edition = "2018"
rust-version = "1.75.0"
rust-version = "1.85.0"
Copy link
Member

@overvenus overvenus May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, does fs2::FileExt require the new rust version?

Copy link
Member Author

@v01dstar v01dstar May 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it was not because of fs2::FileExt. It was because raft-engine as a lib does not have Cargo.lock, and one of the dependencyhalf (required by citron) that requires 1.85+.
This is fine, since TiKV master branch uses 1.85+ nightly

overvenus
overvenus reviewed May 16, 2025
View reviewed changes
@ti-chi-bot ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 17, 2025
@v01dstar v01dstar force-pushed the release-locks-on-drop branch 6 times, most recently from 5f16063 to 3f151a9 Compare May 18, 2025 21:32
@v01dstar
Fix clippy
5ee3c9f 
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
@v01dstar v01dstar force-pushed the release-locks-on-drop branch from 3f151a9 to 5ee3c9f Compare May 18, 2025 21:39
@ti-chi-bot ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels May 19, 2025
zhangjinpeng87
zhangjinpeng87 approved these changes May 19, 2025
View reviewed changes
Copy link
Member

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-chi-bot ti-chi-bot bot added lgtm size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. lgtm size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2025
@v01dstar v01dstar force-pushed the release-locks-on-drop branch from 3da95c8 to 413f0a8 Compare May 20, 2025 05:57
@v01dstar
Update CI
7cacf66 
Signed-off-by: Yang Zhang <yang.zhang@pingcap.com>
@v01dstar v01dstar force-pushed the release-locks-on-drop branch from 413f0a8 to 7cacf66 Compare May 20, 2025 06:15
@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 20, 2025
LykxSassinator
LykxSassinator approved these changes May 20, 2025
View reviewed changes
Cargo.toml
authors = ["The TiKV Project Developers"]
edition = "2018"
rust-version = "1.75.0"
rust-version = "1.85.0"
Copy link
Contributor

@LykxSassinator LykxSassinator May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update the minimum required rust version baseline?

@ti-chi-bot ti-chi-bot bot added the lgtm label May 20, 2025
@ti-chi-bot
Copy link

ti-chi-bot bot commented May 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LykxSassinator, overvenus, zhangjinpeng87

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [LykxSassinator,overvenus,zhangjinpeng87]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label May 20, 2025
@ti-chi-bot
Copy link

ti-chi-bot bot commented May 20, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-05-19 06:41:52.489491272 +0000 UTC m=+354078.745388668: ☑️ agreed by overvenus.
  • 2025-05-19 20:59:04.940051371 +0000 UTC m=+36182.041218593: ☑️ agreed by zhangjinpeng87.
  • 2025-05-20 05:53:36.011343229 +0000 UTC m=+68253.112510451: ✖️🔁 reset by v01dstar.
  • 2025-05-20 07:13:50.131376052 +0000 UTC m=+73067.232543274: ☑️ agreed by overvenus.
  • 2025-05-20 07:14:17.330632501 +0000 UTC m=+73094.431799708: ☑️ agreed by LykxSassinator.

@ti-chi-bot ti-chi-bot bot merged commit 392f5e6 into tikv:master May 20, 2025
7 checks passed
@v01dstar v01dstar mentioned this pull request Jun 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@overvenus overvenus overvenus approved these changes

@LykxSassinator LykxSassinator LykxSassinator approved these changes

@zhangjinpeng87 zhangjinpeng87 zhangjinpeng87 approved these changes

Assignees

No one assigned

Labels

approved dco-signoff: yes Indicates the PR's author has signed the dco. lgtm size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@v01dstar @LykxSassinator @overvenus @zhangjinpeng87