-
Notifications
You must be signed in to change notification settings - Fork 0
Conversation
src/builder_state.rs
Outdated
txn_info.remove_entry(); | ||
} | ||
}); | ||
|
||
// clear the txns from included txns which are older than current timestamp by 3 secs |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 hr?
There was a problem hiding this comment.
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.
There was a problem hiding this 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; | ||
} | ||
|
There was a problem hiding this comment.
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)
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Closes #151