Skip to content
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.

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

Send updates to the new shard during resharding #4256

Merged
merged 5 commits into from
May 30, 2024
Merged

Conversation

ffuugoo
Copy link
Contributor

@ffuugoo ffuugoo commented May 17, 2024

Tracked in #4213.

This PR enables sending update to the created shard and adds basic error handling for failed updates.

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?

@ffuugoo ffuugoo marked this pull request as draft May 17, 2024 11:30
@ffuugoo ffuugoo mentioned this pull request May 17, 2024
39 tasks
Base automatically changed from add-start-resharding-api to dev May 17, 2024 19:17
@ffuugoo ffuugoo force-pushed the resharding-updates branch 2 times, most recently from 0ecbf7a to 83c310a Compare May 23, 2024 13:36
@ffuugoo ffuugoo changed the base branch from dev to cancel-resharding May 23, 2024 13:37
@ffuugoo ffuugoo marked this pull request as ready for review May 23, 2024 13:37
@ffuugoo ffuugoo requested review from timvisee and generall May 23, 2024 13:37
@ffuugoo
Copy link
Contributor Author

ffuugoo commented May 23, 2024

This still feels a bit "hacky", IMO, but I don't want to spend more time on this RN. I propose we merge it, and then improve on it once we started working on the resharding driver.

Comment on lines 394 to 387
// TODO(resharding): 🤔
if current_state == Some(ReplicaState::Resharding) && state == ReplicaState::Dead {
let shard_key = shard_holder
.get_shard_id_to_key_mapping()
.get(&shard_id)
.cloned();

drop(shard_holder);

self.abort_resharding(peer_id, shard_id, shard_key).await?;

// TODO(resharding): Abort all resharding transfers!?

return Ok(());
}
Copy link
Member

Choose a reason for hiding this comment

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

How about doing this in sync_local_state, like we do with shard transfers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you elaborate? abort_resharding does not send any consensus messages. It actually aborts resharding locally. We also abort shard transfers on Dead state in the same way (just a few lines below selected snippet).

Copy link
Member

Choose a reason for hiding this comment

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

👍

I missed that bit.

Where should we produce the abort resharding concencus call though? Should the driver itself be responsible for this?

Copy link
Contributor Author

@ffuugoo ffuugoo May 30, 2024

Choose a reason for hiding this comment

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

Either user aborts explicitly, or the driver (e.g., if one of the transfers failed). When update fails we just propose peer Dead and abort when applying this state change.

lib/collection/src/shards/replica_set/update.rs Outdated Show resolved Hide resolved
@timvisee timvisee force-pushed the cancel-resharding branch 2 times, most recently from 3f7a5e1 to 11c43b2 Compare May 28, 2024 07:22
Base automatically changed from cancel-resharding to dev May 28, 2024 08:36
@ffuugoo ffuugoo marked this pull request as draft May 29, 2024 09:43
@ffuugoo ffuugoo marked this pull request as ready for review May 29, 2024 14:25
@ffuugoo ffuugoo merged commit eeb3731 into dev May 30, 2024
18 checks passed
@ffuugoo ffuugoo deleted the resharding-updates branch May 30, 2024 14:30
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.

None yet

2 participants