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

keep knowledge of ongoing merges across merge pipelines #5633

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

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

use census::{Inventory, TrackedObject as InventoredObject};

pub type TrackedObject<T> = InventoredObject<RecordUnacknoledgedDrop<T>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

}

impl<T> RecordUnacknoledgedDrop<T> {
pub fn acknoledge(&self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo (acknowledge)

}

impl<T> RecordUnacknoledgedDrop<T> {
pub fn acknoledge(&self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn acknoledge(&self) {
pub fn acknowledge_and_drop(self) {

pub type TrackedObject<T> = InventoredObject<RecordUnacknoledgedDrop<T>>;

#[derive(Clone)]
pub struct Tracker<T> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This struct needs a comment explaining its point.

let (sender, _receiver) = channel();
RecordUnacknoledgedDrop {
inner: MaybeUninit::new(value),
acknoledged: true.into(),
Copy link
Contributor

@fulmicoton fulmicoton Jan 22, 2025

Choose a reason for hiding this comment

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

nitpick... (this helps the reader)

Suggested change
acknoledged: true.into(),
acknoledged: AtomicBool::from(true),

/// Return whether it is safe to recreate this tracker.
///
/// A tracker is considered safe to recreate if this is the only instance left,
/// and it conaints no alive object (it may contain dead objects though).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// and it conaints no alive object (it may contain dead objects though).
/// and it contains no alive object (it may contain dead objects though).

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?

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.

Same merge planned twice
2 participants