Skip to content

Make snapshot_manifest methods async#7629

Merged
KShivendu merged 2 commits intodevfrom
snapshot-manifest-async
Nov 27, 2025
Merged

Make snapshot_manifest methods async#7629
KShivendu merged 2 commits intodevfrom
snapshot-manifest-async

Conversation

@ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented Nov 27, 2025

So that they don't block async runtime. 💁‍♀️

All Submissions:

  • Contributions should target the dev branch. Did you create your branch from dev?
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests?
  2. Have you formatted your code locally using cargo +nightly fmt --all command prior to submission?
  3. Have you checked your code using cargo clippy --all --all-features command?

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@coderabbitai

This comment was marked as resolved.

@ffuugoo
Copy link
Contributor Author

ffuugoo commented Nov 27, 2025

Small caveat: I spawn snapshot_manifest on search runtime (cause it's a read-only op on segments) and restore_shard_snapshot on update runtime (cause, well, it's defo not "search"). Given the issues we've seen, I'd say maybe possibly this might have a small potential for a dead/live lock. Maybe.

But I'm also not sure if it would be any better to spawn both tasks on the same runtime.

Copy link
Member

@KShivendu KShivendu left a comment

Choose a reason for hiding this comment

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

Thanks! I wanted to pass down general runtime from toc. But let's go with this as per the 🎲 xD

@KShivendu
Copy link
Member

KShivendu commented Nov 27, 2025

But I'm also not sure if it would be any better to spawn both tasks on the same runtime

I think it should be better. Like deadlocks happens when one thread is waiting on other to free up stuff. But if it's all in the same runtime/thread, deadlock shouldn't happen, right?

@timvisee
Copy link
Member

But I'm also not sure if it would be any better to spawn both tasks on the same runtime

I think it should be better. Like deadlocks happens when one thread is waiting on other to free up stuff. But if it's all in the same runtime/thread, deadlock shouldn't happen, right?

In this case there's a spawn and a blocking spawn. Even within the same runtime they would already be isolated in different 'pools'.

@KShivendu KShivendu merged commit 3daaba2 into dev Nov 27, 2025
15 checks passed
@KShivendu KShivendu deleted the snapshot-manifest-async branch November 27, 2025 15:02
timvisee pushed a commit that referenced this pull request Dec 3, 2025
* Make `snapshot_manifest` methods `async`, so that they don't block async runtime

* Spawn `restore_shard_snapshot` task on `update` runtime instead of "current"
@timvisee timvisee mentioned this pull request Dec 3, 2025
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants