Fix writing aggregation jobs touching GC'ed batches. #2467
Conversation
This issue should only exist in the time-interval query type, as fixed-size is arranged such that aggregation jobs touching a given batch must be GC'ed before the batch. I include a guard to ensure that the new codepath is only taken in the expected case of an already-GC'ed batch for a time-interval query, as otherwise we might drop batch writes if we fell into it unexpectedly.
|
Closes #2464. (though I still need to evaluate the Helper codepath, which I should split out to a separate issue if there is work needed there) I'm not totally happy with this fix as it's something of a hack, but I think it's about the best we can do -- the best other solution discussed was to return an I verified that the new test reproduces the issue by removing the fix & observing the test fail with an underflow error. |
| tx.put_aggregator_task(&leader_task).await?; | ||
| tx.put_client_report(vdaf.borrow(), &first_report).await?; | ||
| tx.put_client_report(vdaf.borrow(), &second_report).await?; |
There was a problem hiding this comment.
nit: I think it's better to call unwrap() here, because then if any of these three puts does fail, it's obvious which one it was. If we propagate the error then it blows up when we unwrap the result from run_unnamed_tx which is less helpful.
There was a problem hiding this comment.
Good call, I've been making this change as I touch code -- it is indeed much better to fail sooner rather than later in tests. I missed these as I adapted a lot of this code from a previously-existing test.
| if !Q::to_batch_interval(batch_identifier) | ||
| .map(|interval| interval.end() < tx.clock().now()) | ||
| .unwrap_or(false) |
There was a problem hiding this comment.
This works fine, but we have self.task in scope, so can't we more directly check the query type?
There was a problem hiding this comment.
I suppose we should check both -- I also want to guard against a time-interval task unexpectedly not finding a batch that shouldn't be GC'ed.
This issue should only exist in the time-interval query type, as fixed-size is arranged such that aggregation jobs touching a given batch must be GC'ed before the batch. I include a guard to ensure that the new codepath is only taken in the expected case of an already-GC'ed batch for a time-interval query, as otherwise we might drop batch writes if we fell into it unexpectedly.