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

Max Cache Duration per Subscription #450

Closed
wants to merge 2 commits into from

Conversation

ianswett
Copy link
Collaborator

@ianswett ianswett commented May 22, 2024

A variant of #446 that is on a Subscription granularity, instead of Object.

Fixes #440

A variant of #446 that is on a Subscription granularity, instead of Object duration.
@suhasHere
Copy link
Collaborator

Per my read of the room, i would prefer #446 as it is gives better flexibility for the application than enforcing it per subscription ( as it applies the same value to all the objects) . Also not sure if there are cases where this can change between the subscriptions either

@VMatrix1900
Copy link

Per my read of the room, i would prefer #446 as it is gives better flexibility for the application than enforcing it per subscription ( as it applies the same value to all the objects) . Also not sure if there are cases where this can change between the subscriptions either

+1, Cache Duration per object is more flexible.

@@ -1414,11 +1415,19 @@ longer valid. A value of 0 indicates that the subscription does not expire
or expires at an unknown time. Expires is advisory and a subscription can
end prior to the expiry time or last longer.

* Max Cache Duration: The maximum length of time in milliseconds a caching
relay MAY cache the subscription for. A value of 0 indicates there is no limit.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this have value for non-relays (eg: subscriber side caches?)

Copy link
Contributor

@fluffy fluffy left a comment

Choose a reason for hiding this comment

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

I don't understand how the relay would know the answer to this at the time the subscribe happened, and I don't understand what use case the applications would need to get this from the relay instead of some other way like the manifest.

ianswett added a commit that referenced this pull request Jun 18, 2024
A tweaked version of #448 

Closes #450 

I'll note that we might want to add params to TRACK_STATUS and allow this to be returned from that as well, which would allow a relay to keep it in its cache.
Copy link
Contributor

@fluffy fluffy left a comment

Choose a reason for hiding this comment

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

I think this was written before our current understanding of caching object not subscriptions so I have proposed a few updates but if it is meant to still be about refreshing subscriptions, then ignore all my comments.

@@ -1519,6 +1519,7 @@ SUBSCRIBE_OK
{
Subscribe ID (i),
Expires (i),
Max Cache Duration (i),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be slightly better as an optional parameter but clearly not a big deal either way

@@ -1533,6 +1534,13 @@ longer valid. A value of 0 indicates that the subscription does not expire
or expires at an unknown time. Expires is advisory and a subscription can
end prior to the expiry time or last longer.

* Max Cache Duration: The maximum length of time in milliseconds a caching
relay MAY cache the subscription for. A value of 0 indicates there is no limit.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is the objects that are cached not the subscription so I think this needs to say "MAY cache the object for"

* Max Cache Duration: The maximum length of time in milliseconds a caching
relay MAY cache the subscription for. A value of 0 indicates there is no limit.
The relay MUST NOT begin sending Objects for a new subscription from cache if
it's older than Max Cache Duration. When a relay responds with SUBSCRIBE_OK,
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to be clear the time starts at when the relay started to receive the object.

The relay MUST NOT begin sending Objects for a new subscription from cache if
it's older than Max Cache Duration. When a relay responds with SUBSCRIBE_OK,
the amount of time since the corresponding upstream subscription was made is
subtracted when sending the Max Cache Duration.
Copy link
Contributor

Choose a reason for hiding this comment

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

This sentence does not make sense if we are caching objects not subscriptions. Would just remove it.

@ianswett
Copy link
Collaborator Author

Closing this in favor of #469

@ianswett ianswett closed this Jul 25, 2024
ianswett added a commit that referenced this pull request Aug 5, 2024
A tweaked version of #448 

Fixes #440
Fixes #249

Closes #450 
Closes #448

I'll note that we might want to add params to TRACK_STATUS and allow
this to be returned from that as well, which would allow a relay to keep
it in its cache.

Attempts to write down where we ended up at the end of the June 12th
interim.
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.

Need expiry time / time to live
5 participants