Remove ReportsProcessed DO #433
Conversation
mendess
requested review from
captain-mota,
chris-wood and
cjpatton
as code owners
mendess
force-pushed
the
mendess/reports-processed
branch
2 times, most recently
from
a9aafd8 to
1066290
Compare
There was a problem hiding this comment.
Great to see that we can basically rip this out. However I think I see at least one regression: The Leader no longer checks for replays in time to reject the reports during an aggregation job.
Also, and this is somewhat separate so feel free to punt: The ReportsProcessed DO instances are self-cleaning in the sense that, on creation, an alarm is set that deletes the instance some time later. Now would be a good time to implement this for AggregateStore as well, since we mainly just need to copy-paste the code from the reports_processed module to aggregate_store.
mendess
force-pushed
the
mendess/reports-processed
branch
2 times, most recently
from
30f35ab to
f6a83f3
Compare
mendess
force-pushed
the
mendess/reports-processed
branch
from
f6a83f3 to
9571ad1
Compare
mendess
force-pushed
the
mendess/reports-processed
branch
from
9571ad1 to
15f5930
Compare
There was a problem hiding this comment.
Looks great overall, but I think this needs a few minor changes. I've also bumped some comments that we missed in the previous round.
Also, I think we should delete this outdated comment we noticed on Tuesday (this caused some confusion about if/when the Leader needs to check for replays): https://github.com/cloudflare/daphne/blob/main/daphne_worker/src/roles/leader.rs#L105-L109
mendess
force-pushed
the
mendess/reports-processed
branch
from
d4d378c to
8231f9e
Compare
mendess
force-pushed
the
mendess/reports-processed
branch
from
8231f9e to
ec84c0b
Compare
No description provided.