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

Add an algorithm of storage maintenance (IG Cap). #859

Merged
merged 10 commits into from
Oct 25, 2023

Conversation

qingxinwu
Copy link
Collaborator

@qingxinwu qingxinwu commented Oct 12, 2023

It includes several IG caps.

Also link "user agent" to Infra's [=user agent=].


Preview | Diff

It includes several IG caps.
@qingxinwu qingxinwu added the spec Relates to the spec label Oct 12, 2023
@qingxinwu qingxinwu requested a review from JensenPaul October 12, 2023 17:52
@qingxinwu qingxinwu changed the title Add an algorithm of storage maintenance. Add an algorithm of storage maintenance (IG Cap). Oct 12, 2023
@qingxinwu
Copy link
Collaborator Author

@orrb1 PTAL. Thanks! Not sure why I cannot add you to the reviewers.

spec.bs Show resolved Hide resolved
@qingxinwu qingxinwu requested a review from domfarolino October 16, 2023 15:36
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 Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated Show resolved Hide resolved
@qingxinwu qingxinwu requested a review from domfarolino October 16, 2023 22:04
Copy link
Collaborator

@orrb1 orrb1 left a comment

Choose a reason for hiding this comment

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

Thank you, @qingxinwu!

spec.bs Outdated Show resolved Hide resolved
spec.bs Show resolved Hide resolved
spec.bs 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
spec.bs Outdated Show resolved Hide resolved
spec.bs Outdated

There is a job that periodically [=performs storage maintenance=] on the [=user agent=]'s
[=interest group set=]. It performs operations such as [=list/removing=] expired or excess
[=interest groups=]. An [=interest group set=] must have no more than:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@domfarolino Should we define these numbers as "Vendor (or Implementation) Specific Values" as some specs do? I'd think so, since we're fine with vendors choose their own numbers for these, instead of sticking with our pick.

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

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

LGTM % discussion in https://github.com/WICG/turtledove/pull/859/files#r1357262550, which I'd like to understand better.

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

qingxinwu commented Oct 20, 2023

LGTM % discussion in https://github.com/WICG/turtledove/pull/859/files#r1357262550, which I'd like to understand better.

How about this comment about defining the limits as "vendor specific values"? We also want to provide our limits as an example for people to start with if they want, so wondering what would be the best format/wording (e.g., an "Example" with our numbers, or "Note" for that, etc.)

@domfarolino
Copy link
Collaborator

How about #859 (comment) about defining the limits as "vendor specific values"? We also want to provide our limits as an example for people to start with if they want, so wondering what would be the best format/wording (e.g., an "Example" with our numbers, or "Note" for that, etc.)

Hmm, so there seems to be two places where we might want to introduce implementation-defined "looseness" around IG size, and I'm not sure what the difference is:

Are both of these desirable? Or are both of these just different ways of achieving the same thing?

@qingxinwu
Copy link
Collaborator Author

qingxinwu commented Oct 23, 2023

How about #859 (comment) about defining the limits as "vendor specific values"? We also want to provide our limits as an example for people to start with if they want, so wondering what would be the best format/wording (e.g., an "Example" with our numbers, or "Note" for that, etc.)

Hmm, so there seems to be two places where we might want to introduce implementation-defined "looseness" around IG size, and I'm not sure what the difference is:

Are both of these desirable? Or are both of these just different ways of achieving the same thing?

They're different things. I'll keep the size estimation part under the other comment (and it turns out that's no longer a problem).

Here, it's about the size limitation an implementation/vendor might want to put on stored IGs, such as total maximum sizes of IGs per owner. We chose our numbers for those limitations, but we'd like to allow other vendors to choose their limitations as they want if their browser can handle more/less limits (e.g., allow storing more IGs per owner, give higher storage limit per owner, etc.), and our numbers are examples and start point for them. If they'd like to use the same numbers, that's fine as well.

@qingxinwu qingxinwu requested a review from domfarolino October 24, 2023 17:40
@domfarolino
Copy link
Collaborator

We chose our numbers for those limitations, but we'd like to allow other vendors to choose their limitations as they want if their browser can handle more/less limits (e.g., allow storing more IGs per owner, give higher storage limit per owner, etc.), and our numbers are examples and start point for them. If they'd like to use the same numbers, that's fine as well.

OK thanks for clarifying. In that case, I'd say something like:

Implementations may define their own values for the below constants, however we supply the below values as a starting point, inspired by what the initial implementation of this specification uses:

That can go above the definition of the constants you define. Does that make sense?

@qingxinwu
Copy link
Collaborator Author

We chose our numbers for those limitations, but we'd like to allow other vendors to choose their limitations as they want if their browser can handle more/less limits (e.g., allow storing more IGs per owner, give higher storage limit per owner, etc.), and our numbers are examples and start point for them. If they'd like to use the same numbers, that's fine as well.

OK thanks for clarifying. In that case, I'd say something like:

Implementations may define their own values for the below constants, however we supply the below values as a starting point, inspired by what the initial implementation of this specification uses:

That can go above the definition of the constants you define. Does that make sense?

Done. That sounds great. Thanks

@JensenPaul
Copy link
Collaborator

I gave this a quick review and it LGTM.

@JensenPaul JensenPaul merged commit 07960ec into WICG:main Oct 25, 2023
1 check passed
github-actions bot added a commit that referenced this pull request Oct 25, 2023
SHA: 07960ec
Reason: push, by JensenPaul

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@qingxinwu qingxinwu deleted the igcap branch October 25, 2023 17:46
github-actions bot added a commit to qingxinwu/turtledove that referenced this pull request Oct 26, 2023
SHA: 07960ec
Reason: push, by qingxinwu

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
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants