Implement the aggregation flow as in draft07 #409
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjpatton
commented
Oct 13, 2023
cjpatton
force-pushed
the
cjpatton/dap07/6
branch
from
d0623cf to
9188356
Compare
cjpatton
commented
Oct 13, 2023
cjpatton
commented
Oct 13, 2023
cjpatton
force-pushed
the
cjpatton/dap07/6
branch
2 times, most recently
from
b37c805 to
4ed73a1
Compare
cjpatton
changed the base branch from
main
to
cjpatton/try-put-agg-share-span
cjpatton
force-pushed
the
cjpatton/dap07/6
branch
from
ad21d31 to
7423f26
Compare
cjpatton
requested review from
armfazh,
bhalleycf,
captain-mota,
chris-wood and
mendess
as code owners
mendess
reviewed
Oct 17, 2023
mendess
reviewed
Oct 17, 2023
cjpatton
force-pushed
the
cjpatton/try-put-agg-share-span
branch
from
f109916 to
d0ac903
Compare
cjpatton
force-pushed
the
cjpatton/dap07/6
branch
from
7423f26 to
cce9de6
Compare
cjpatton
changed the base branch from
cjpatton/try-put-agg-share-span
to
mendess/fix-do-consistency-issue
cjpatton
commented
Oct 19, 2023
cjpatton
commented
Oct 19, 2023
cjpatton
commented
Oct 19, 2023
| (state, agg_job_init_req) | ||
| } | ||
| DapLeaderAggregationJobTransition::Finished(agg_span) | ||
| if agg_span.report_count() == 0 => |
There was a problem hiding this comment.
I removed Skip because it feels a bit redundant. But the fact that we need this if statement here makes me wonder if we really want an empty agg span to distinguish between these branches.
There was a problem hiding this comment.
If the agg span is empty, then we quite the agg job. Otherwise, if the agg span is non-empty, we send the next request for the agg job.
cjpatton
commented
Oct 19, 2023
| agg_span | ||
| } else { | ||
| return Ok(0); | ||
| } |
There was a problem hiding this comment.
Similar comment here.
cjpatton
commented
Oct 19, 2023
cjpatton
force-pushed
the
cjpatton/dap07/6
branch
from
cce9de6 to
0daf9e8
Compare
mendess
force-pushed
the
mendess/fix-do-consistency-issue
branch
from
1094133 to
90de281
Compare
mendess
requested changes
Oct 25, 2023
cjpatton
force-pushed
the
cjpatton/dap07/6
branch
2 times, most recently
from
2bfd1ae to
a9f8971
Compare
mendess
approved these changes
Oct 27, 2023
In draft07, the Leader sends its prep share in the AggregationJobInitReq; the Helper computes its prep share, computes the prep message, and commits. For 1-round VDAFs, the flow is complete after one request. The main goal of this change is to implement this new control flow. Other changes: * Clean up and unify data structures for aggregation job state. * Rename the `aggregation_job_continue_repeats_due_to_replays` metric (we need to do an analogous thing during AggregationJobInitReq processing). * Regression: Remove the `handle_agg_job_req_init_expired_task` tests. Ideally we'd like to keep these, but it's difficult to do so without major refactoring. * Ensure Leader handles an `Err` from `try_put_agg_share_span()` as fatal. Work-in-progress: The code is not yet wire-compatible with draft07.
cjpatton
force-pushed
the
cjpatton/dap07/6
branch
from
027fafb to
2834966
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stacked on #412.
Partially addresses #350.
In draft07, the Leader sends its prep share in the
AggregationJobInitReq; the Helper computes its prep share, computes the
prep message, and commits. For 1-round VDAFs, the flow is complete after
one request.
The main goal of this change is to implement this new control flow.
Other changes:
Clean up and unify data structures for aggregation job state.
Rename the
aggregation_job_continue_repeats_due_to_replaysmetric(we need to do an analogous thing during AggregationJobInitReq
processing).
Regression: Remove the
handle_agg_job_req_init_expired_tasktests.Ideally we'd like to keep these, but it's difficult to do so without
major refactoring.
Work-in-progress: The code is not yet wire-compatible with draft07.