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

Spec: Aggregation coordinator choice #106

Merged
merged 8 commits into from Oct 24, 2023
Merged

Spec: Aggregation coordinator choice #106

merged 8 commits into from Oct 24, 2023

Conversation

alexmturner
Copy link
Collaborator

@alexmturner alexmturner commented Oct 11, 2023

Adds a mechanism for choosing between multiple aggregation coordinators. See also the explainer update:
#101.

To simplify this spec change, the Protected Audience integration's get batching scope steps are modified to be lazy.


Preview | Diff

Adds a mechanism for choosing between multiple aggregation coordinators.
See also the explainer update:
#101
@alexmturner
Copy link
Collaborator Author

@linnan-github, could you PTAL? Thanks!

cc @yoavweiss

spec.bs Outdated
: <dfn>context ID</dfn>
:: A [=string=] or null
: <dfn>queued</dfn>
:: A [=boolean=]

</dl>

<h3 dfn-type=dfn>Aggregation coordinator</h3>
An aggregation coordinator is an [=origin=] that the [=allowed aggregation

Choose a reason for hiding this comment

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

Shall we use suitable origin to align with ARA?
https://wicg.github.io/attribution-reporting-api/#aggregation-coordinator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added an issue here -- we don't have the 'suitable' concept in PAA yet. It might make sense to switch over, but I think currently most of the logic does not assume that the scheme is either http or https.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@alexmturner alexmturner left a comment

Choose a reason for hiding this comment

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

Thanks!

spec.bs Outdated
: <dfn>context ID</dfn>
:: A [=string=] or null
: <dfn>queued</dfn>
:: A [=boolean=]

</dl>

<h3 dfn-type=dfn>Aggregation coordinator</h3>
An aggregation coordinator is an [=origin=] that the [=allowed aggregation
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added an issue here -- we don't have the 'suitable' concept in PAA yet. It might make sense to switch over, but I think currently most of the logic does not assume that the scheme is either http or https.

spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@alexmturner
Copy link
Collaborator Author

@qingxinwu @xyaoinum could you PTAL too? (Or reassign) Thanks!

Copy link

@xyaoinum xyaoinum left a comment

Choose a reason for hiding this comment

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

changes related to shared-storage LGTM

@qingxinwu
Copy link

I'll be able to take a look later tomorrow (Friday), if that's not too late.

@alexmturner
Copy link
Collaborator Author

Tomorrow works! Thanks both

aarongable pushed a commit to chromium/chromium that referenced this pull request Oct 20, 2023
Adds a new aggregation_coordinator_origin field to the Private
Aggregation layer. This field is validated and then sent on to the
aggregation_service layer. This change is not web visible as the
integrations with Shared Storage and Protected Audience will be added in
separate cls.

See the related spec change:
patcg-individual-drafts/private-aggregation-api#106

Note that this also modifies behavior of the database to only skip
reports that do not deserialize instead of skipping all reports if
there is any that do not deserialize. This may change behavior in case
of database corruption.

Bug: 1481254
Change-Id: Ie90eafc208f94ffd98ad95642b8f8c9b0af85fac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4885966
Reviewed-by: Yao Xiao <yaoxia@chromium.org>
Reviewed-by: Russ Hamilton <behamilton@google.com>
Commit-Queue: Alex Turner <alexmt@chromium.org>
Reviewed-by: Nan Lin <linnan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1212827}
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@qingxinwu
Copy link

Hmm didn't find a place to approve, maybe I cannot. But LGTM

@alexmturner
Copy link
Collaborator Author

Thanks!

@alexmturner alexmturner merged commit 55019b5 into main Oct 24, 2023
1 check passed
@alexmturner alexmturner deleted the multi-cloud-spec branch October 24, 2023 16:30
github-actions bot added a commit that referenced this pull request Oct 24, 2023
SHA: 55019b5
Reason: push, by alexmturner

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants