Skip to content
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

Make the checksum optional #511

Closed
wants to merge 1 commit into from
Closed

Make the checksum optional #511

wants to merge 1 commit into from

Conversation

chris-wood
Copy link
Collaborator

Closes #446

Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle I have no objection to the checksum being optional, but I think we should figure out a way to signal to the Helper at aggregation time that the the checksum will be requested later at collection time. I explain below.

The checksum is designed so that the order of reports doesn't matter: if both Aggregators have the same set of reports in the batch, then the checksums will match and collection will succeed. This means that we can either compute it as we go at aggregation time or wait until collection time and compute it all at once. The former is strictly better because we already have the report IDs in memory during aggregation time, whereas if we wait until collection time, we'd have to make a bunch of database queries to look up the report IDs in the batch.

Computing it aggregation time is always better I think. If the Helper does not know whether the checksum will be requested, I suspect it'll always want to compute it ahead of time just in case.

We can avoid this by signaling ahead of time that the checksum will be requested. One way to do this would be to add the signal to the task config.

@chris-wood
Copy link
Collaborator Author

Putting that signal in the task config seems like it'd work!

Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to "request changes" while we wait on discussion. (This PR needs work as-is, but it could work with minor changes.)

@chris-wood chris-wood closed this Nov 7, 2023
@chris-wood
Copy link
Collaborator Author

Closing per discussion at IETF 118.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cheaper checksum
2 participants