feat(discovery-core): add iobudget batching, cursor types, and new cost constants - #459
Conversation
9207333 to
ab8658c
Compare
ab8658c to
680b6bc
Compare
848fff3 to
3ae8597
Compare
680b6bc to
8cf9538
Compare
3ae8597 to
210b963
Compare
210b963 to
d666926
Compare
8cf9538 to
7d40aef
Compare
7d40aef to
ab28315
Compare
d666926 to
0f0ab25
Compare
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @m-kus).
crates/discovery-core/src/io_budget.rs line 73 at r1 (raw file):
if cost_per_item == 0 || max_items == 0 { return 0; }
cost_per_item=0 means you can consume as many items as you want.
If you don't want to support that, please assert that cost_per_item != 0
Code quote:
pub fn consume_up_to(&self, max_items: usize, cost_per_item: usize) -> usize {
if cost_per_item == 0 || max_items == 0 {
return 0;
}crates/discovery-core/src/io_budget.rs line 86 at r1 (raw file):
Some(current - num_items * cost_per_item) } })
Suggestion, a bit clearer
Suggestion:
let max_items_cap = max_items.min(self.batch_budget / cost_per_item);
if max_items_cap == 0 {
return 0;
}
self.remaining
.fetch_update(Ordering::SeqCst, Ordering::SeqCst, |current| {
let num_items = (current / cost_per_item).min(max_items_cap);
let count = num_items * cost_per_item;
current.checked_sub(count)
})
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @m-kus).
6399937 to
f30ad55
Compare
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @m-kus).
crates/discovery-core/src/io_budget.rs line 38 at r2 (raw file):
/// Sets the batch budget (max budget units per batch). Returns `self` for chaining. pub fn with_batch_budget(mut self, batch_budget: usize) -> Self { self.batch_budget = batch_budget;
Thinking about this again, why do we need the batch budget here?
Why not as a config of the state reader? batch_size merely limits the size of a single read request. I don't see a problem in handling bigger requests, just in chunks of that size, iteratively.
00f57ec to
32f582c
Compare
f30ad55 to
06bd4d7
Compare
06bd4d7 to
fbe6761
Compare
m-kus
left a comment
There was a problem hiding this comment.
@m-kus made 1 comment.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware).
crates/discovery-core/src/io_budget.rs line 38 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Thinking about this again, why do we need the batch budget here?
Why not as a config of the state reader?
batch_sizemerely limits the size of a single read request. I don't see a problem in handling bigger requests, just in chunks of that size, iteratively.
Done.
fbe6761 to
193a70c
Compare
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on m-kus).
Yoni-Starkware
left a comment
There was a problem hiding this comment.
@Yoni-Starkware made 1 comment and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on m-kus).
crates/discovery-core/src/io_budget.rs line 38 at r2 (raw file):
Previously, m-kus (Michael Zaikin) wrote…
Done.
Thanks!
193a70c to
8f63cdc
Compare

TL;DR
Added cursor types and improved I/O budget functionality to support batch discovery operations.
What changed?
DiscoveryCursor,ChannelCursor,SubchannelCursor) to track progress across paginated discovery callsIoBudgetwith batch budget functionality and a newconsume_up_tomethod for more efficient resource allocationfuturescrate dependency to support async operationsThis change is