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

Introduce stream_ttl #222

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

TusharR-google
Copy link

@TusharR-google TusharR-google changed the title Introduce push_stream_ttl in openid-sharedsignals-framework-1_0.md Introduce push_stream_ttl Dec 3, 2024
@TusharR-google TusharR-google marked this pull request as ready for review December 3, 2024 17:59
@TusharR-google TusharR-google requested a review from a team as a code owner December 3, 2024 17:59
Copy link
Contributor

@appsdesh appsdesh left a comment

Choose a reason for hiding this comment

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

Push and Pull are delivery mechanisms defined outside the SSF spec. We should not introduce new aspects specifically targeting a single delivery mechanism in the SSF spec

@FragLegs
Copy link
Contributor

FragLegs commented Dec 3, 2024

Push and Pull are delivery mechanisms defined outside the SSF spec. We should not introduce new aspects specifically targeting a single delivery mechanism in the SSF spec

I agree. Can we just make this stream_ttl so that it applies to any stream regardless of delivery method?

@TusharR-google
Copy link
Author

Push and Pull are delivery mechanisms defined outside the SSF spec. We should not introduce new aspects specifically targeting a single delivery mechanism in the SSF spec

Thanks! The same concept can be applied to PULL streams too, let me reword this as simply stream_tll.


> OPTIONAL. The lifetime of a PUSH stream in seconds, after which the Transmitter MAY either pause or disable the stream if it has not received any Receiver-initiated communication in that duration.
If the Transmitter decides to update the stream, it MUST send a Stream Updated Event to the Receiver as described in {{status}}.
If the Receiver calls any endpoint in the Event Stream Management API ({{management}}), the Transmitter must refresh the TTL of that particular stream.
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want to be explicit that contact from the receiver does not automatically move the stream out of a paused/disabled state caused by a TTL timeout.

Copy link
Author

Choose a reason for hiding this comment

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

Should we leave it up to the transmitter? It might complicate the spec if paused/disabled states were treated differently based on whether they were caused by a TTL timeout (Tx not allowed to automatically change the state), vs other causes like Rx requesting the pause (Tx allowed to do whatever).

@tulshi tulshi linked an issue Dec 3, 2024 that may be closed by this pull request
@TusharR-google TusharR-google changed the title Introduce push_stream_ttl Introduce stream_ttl Dec 3, 2024
@@ -601,6 +618,13 @@ default_subjects
to be transmitted. The Receiver MAY remove subjects added this way via the
`remove_subject_endpoint`.

stream_ttl

> OPTIONAL. The lifetime of a stream in seconds, after which the Transmitter MAY either pause or disable the stream if it has not received any Receiver-initiated communication in that duration.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Receiver initiated" needs to be clarified, IMO. Does it differ for poll and push streams? Is the appropriate response code from the receiver to a push from a Tx sufficient?

Copy link
Author

Choose a reason for hiding this comment

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

Clarified the definition of "receiver-initiated" communication.

@@ -601,6 +618,13 @@ default_subjects
to be transmitted. The Receiver MAY remove subjects added this way via the
`remove_subject_endpoint`.

stream_ttl

Choose a reason for hiding this comment

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

can we make it stream_ttl_seconds? will save people working with the api a documentation lookup

Copy link
Author

Choose a reason for hiding this comment

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

Most of the other names in the this/similar spec do not seem to use a suffix, regardless of units. E.g. min_verification_interval

So we could keep it as stream_ttl for consistency.

@FragLegs FragLegs added the v1Final Issues that must be fixed before we propose a spec to become a v1 final spec. label Jan 24, 2025
@@ -85,6 +91,17 @@ normative:
target: https://tools.ietf.org/html/rfc6749#section-4.4
title: The OAuth 2.0 Authorization Framework - Client Credentials Grant

EXPIRES_IN:
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need to describe an RFC. You can just add its number and the kramdown tool will do the rest.

@@ -929,6 +946,19 @@ description
This is useful in multi-stream systems to identify the stream for human actors. The
transmitter MAY truncate the string beyond an allowed max length.

stream_ttl

> **Transmitter-Supplied**, OPTIONAL. The refreshable lifetime of the stream in seconds, after which the Transmitter MAY either pause or disable the stream if it has not received any Receiver-initiated communication (defined below) in that duration. The syntax is the same as that of {{EXPIRES_IN}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to expand the acronym "TTL" somewhere in the description. You say it precisely in the first sentence, but you just don't say what "TTL" stands for. It'll help clarify the name even though most folks will know what it is.


> **Transmitter-Supplied**, OPTIONAL. The refreshable lifetime of the stream in seconds, after which the Transmitter MAY either pause or disable the stream if it has not received any Receiver-initiated communication (defined below) in that duration. The syntax is the same as that of {{EXPIRES_IN}}.
>
> For PUSH streams, the Transmitter MUST refresh the TTL whenever:
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to write this as: "For streams created with the PUSH {{RFC8935}} delivery method..."

@@ -929,6 +946,19 @@ description
This is useful in multi-stream systems to identify the stream for human actors. The
transmitter MAY truncate the string beyond an allowed max length.

stream_ttl

> **Transmitter-Supplied**, OPTIONAL. The refreshable lifetime of the stream in seconds, after which the Transmitter MAY either pause or disable the stream if it has not received any Receiver-initiated communication (defined below) in that duration. The syntax is the same as that of {{EXPIRES_IN}}.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the right behavior "pause or disable", or just "delete"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1Final Issues that must be fixed before we propose a spec to become a v1 final spec.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permanence of streams
5 participants