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

Remove changes that failed to process from seen map #230

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

somtochiama
Copy link
Contributor

No description provided.

Copy link
Member

@jeromegn jeromegn left a comment

Choose a reason for hiding this comment

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

Replacing the Mutex with simpler logic should make a smaller, more performant, code change.

error!("could not process multiple changes: {e}");
for change in changes {
let v = *change.0.versions().start();
seen_mutex.lock().remove(&(change.0.actor_id, v));
Copy link
Member

Choose a reason for hiding this comment

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

Adding a mutex here is probably a bit harder than changing the return value of the future and handling it in the loop, in the tokio::select!.

For example, you could return BTreeMap<ActorId, RangeInclusiveSet<Version>> (or something) from the future. It would represent the data to remove from the seen map. You'd only do this if the function errored, else you return an empty BTreeMap (does not allocate).

@@ -461,11 +473,8 @@ pub async fn handle_changes(

// process these first, we don't care about the result,
// but we need to drain it to free up concurrency
res = join_set.join_next(), if !join_set.is_empty() => {
_ = join_set.join_next(), if !join_set.is_empty() => {
Copy link
Member

Choose a reason for hiding this comment

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

Here, you'd handle the map, remove every element present in it, from seen.

@somtochiama somtochiama requested a review from jeromegn July 2, 2024 21:23
@jeromegn jeromegn merged commit cccfad8 into main Jul 2, 2024
3 of 4 checks passed
@jeromegn jeromegn deleted the remove-changes-on-err branch July 2, 2024 23:24
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.

2 participants