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.

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

keep knowledge of ongoing merges across merge pipelines #5633

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

keep knowledge of ongoing merges across merge pipelines #5633

wants to merge 11 commits into from
+529 −49

Conversation

trinity-1686a
Copy link
Contributor

Description

fix #5588

How was this PR tested?

added a test reproducing what was failing before, and making sure it passes now

@trinity-1686a
Copy link
Contributor Author

looks like this made actors::merge_executor::tests::test_delete_all very flaky. I suspect this is because when no split is published, the merge might not get acknowledged properly to the Tracker

@guilload guilload requested a review from fulmicoton January 17, 2025 18:43
fulmicoton
fulmicoton suggested changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@fulmicoton fulmicoton left a comment

Choose a reason for hiding this comment

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

can you squash the commits and have a good commit message describing your solution?

@trinity-1686a
Copy link
Contributor Author

currently this makes one test very flaky (fails more often than not) and i haven't figured out why yet. i'd rather fix that before merging

fulmicoton
fulmicoton reviewed Jan 27, 2025
View reviewed changes
quickwit/quickwit-janitor/src/actors/delete_task_planner.rs Outdated
@@ -88,7 +89,7 @@ pub struct DeleteTaskPlanner {
/// a merge operation is dropped after the publish of the split that underwent
/// the delete operation.
/// The inventory is used to avoid sending twice the same delete operation.
ongoing_delete_operations_inventory: Inventory<MergeOperation>,
ongoing_delete_operations_inventory: Inventory<RecordUnacknoledgedDrop<MergeOperation>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

typo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fulmicoton fulmicoton fulmicoton requested changes

@guilload guilload Awaiting requested review from guilload

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Same merge planned twice
2 participants
@trinity-1686a @fulmicoton