Skip to content
This repository has been archived by the owner on Dec 3, 2024. It is now read-only.

GC included txns #152

Merged
merged 4 commits into from
May 23, 2024
Merged

GC included txns #152

merged 4 commits into from
May 23, 2024

Conversation

move47
Copy link
Contributor

@move47 move47 commented May 22, 2024

Closes #151

@move47 move47 requested a review from nyospe May 22, 2024 15:10
txn_info.remove_entry();
}
});

// clear the txns from included txns which are older than current timestamp by 3 secs
Copy link
Contributor

Choose a reason for hiding this comment

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

What? No. I meant, we could clear after... days? Hours, maybe, at the smallest increment.

Copy link
Contributor Author

@move47 move47 May 22, 2024

Choose a reason for hiding this comment

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

2 hr?

Copy link
Contributor

@nyospe nyospe May 22, 2024

Choose a reason for hiding this comment

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

We can do 2 hours. But... maybe let's try this instead: https://espresso.zulipchat.com/#narrow/stream/419378-Block-Builder/topic/Staging.20performance.20issues/near/440153803 with the <unit of time> configurable, defaulting to 1 hour.

@move47 move47 changed the title GC included txns based on timestamp GC included txns May 22, 2024
Copy link
Contributor

@nyospe nyospe left a comment

Choose a reason for hiding this comment

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

One purely stylistic request.

Otherwise, looks good.

next_txn_garbage_collect_time =
self.next_txn_garbage_collect_time + self.txn_garbage_collect_duration;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

More idiomatic as:

let (included_txns, included_txns_old, included_txns_expiring, next_txn_garbage_collect_time) = if Instant::now() >= self.next_txn_garbage_collect_time {
            (HashSet::new(), self.included_txns.clone(), self.included_txns_old.clone(), Instant::now() + self.txn_garbage_collect_duration)
} else {
            (self.included_txns.clone(), self.included_txns_old.clone(), self.included_txns_expiring.clone(), self. next_txn_garbage_collect_time)
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@nyospe nyospe left a comment

Choose a reason for hiding this comment

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

LGTM

@move47 move47 merged commit ed5cc3c into main May 23, 2024
5 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Garbage Collect Included Txns
2 participants