-
Notifications
You must be signed in to change notification settings - Fork 15
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
Remove multi-collection. #3481
Remove multi-collection. #3481
Conversation
d3a0c10
to
3264784
Compare
Part of #3436. |
3264784
to
9591330
Compare
(apologies, a couple of force-pushes to clean up formatting & a few merge issues with the rebase onto the renaming PR) |
DAP-11 removed the ability to collect a batch more than once (with differing aggregation parameters). This commit also includes related changes to a few messages, and removes the max_batch_query_count task parameter.
9591330
to
cbef95e
Compare
|
||
/// Batch identifier, encoded with base64url | ||
#[clap( | ||
long, | ||
conflicts_with_all = ["batch_interval_start", "batch_interval_duration", "current_batch"], | ||
help_heading = "Collect Request Parameters (Leader Selected)", | ||
)] | ||
batch_id: Option<BatchId>, | ||
/// Have the aggregator select a batch that has not yet been collected | ||
#[clap( | ||
long, | ||
action = ArgAction::SetTrue, | ||
conflicts_with_all = ["batch_interval_start", "batch_interval_duration", "batch_id"], | ||
help_heading = "Collect Request Parameters (Leader Selected)", | ||
)] | ||
current_batch: bool, |
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.
Hmm, so now without explicit --batch-interval-{start,duration}
, we automatically assume we're operating on a fixed size task.
I suppose that's fine. Maybe we can think of a better way to clean up this CLI for this, but I don't think its important.
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.
Yep, that's correct. I thought about this for a bit & came up with the same conclusion you did. We could introduce something like a --leader-selected
flag to indicate a leader-selected query, but I'm not fully sure it's necessary.
DAP-11 removed the ability to collect a batch more than once (with differing aggregation parameters). This commit also includes related changes to a few messages, and removes the max_batch_query_count task parameter.