Skip to content

Clarify wording on SUBSCRIBE/GET to a non-existent path. #144

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.

Sign up for GitHub

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

Open
wenovus wants to merge 2 commits into
base: master
Choose a base branch
from
+7 −3
Open

Clarify wording on SUBSCRIBE/GET to a non-existent path. #144

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7a02f7
Clarify wording on SUBSCRIBE/GET to a non-existent path.
wenovus Sep 10, 2021
83f93c9
Clarify that sync_response is always expected
wenovus Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions rpc/gnmi/gnmi-specification.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,7 +917,9 @@ The `GetResponse` message consists of:
and hence MUST NOT collapse data from multiple paths into a single
`Notification` within the response. The `timestamp` field of the
`Notification` message MUST be set to the time at which the target's
snapshot of the relevant path was taken.
snapshot of the relevant path was taken. If the path is syntactically valid
but does not (yet) exist, then the `update` field of the `Notification` MUST
Copy link
Copy Markdown
Member

@robshakir robshakir Feb 22, 2022

Choose a reason for hiding this comment

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

I think here the target should respond with an error using the NotFound status code. This seems (to me) to be the most explicit thing to do when a client is explicitly asking for a very particular path.

Copy link
Copy Markdown
Contributor Author

@wenovus wenovus Feb 26, 2022

Choose a reason for hiding this comment

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

Wouldn't this deviate from Subscribe where no error is produced?

There is no requirement that the path specified in the message must exist within the current data tree on the server. While the path within the subscription SHOULD be a valid path within the set of schema modules that the target supports, subscribing to any syntactically valid path within such modules MUST be allowed. In the case that a particular path does not (yet) exist, the target MUST NOT close the RPC, and instead should continue to monitor for the existence of the path, and transmit telemetry updates should it exist in the future.

be empty.
* `extension` - a repeated field used to carry gNMI extensions, as per the
description in [Section 2.7](#27-extensions-to-gnmi).

Expand Down Expand Up @@ -1400,8 +1402,10 @@ the current data tree on the server. While the path within the subscription
SHOULD be a valid path within the set of schema modules that the target
supports, subscribing to any syntactically valid path within such modules MUST
be allowed. In the case that a particular path does not (yet) exist, the target
MUST NOT close the RPC, and instead should continue to monitor for the existence
of the path, and transmit telemetry updates should it exist in the future.
would send just a `sync_response` for `ONCE` and `POLL` subscriptions, but MUST
Copy link
Copy Markdown

@dan-lepage dan-lepage Sep 13, 2021

Choose a reason for hiding this comment

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

I think the word "would" ought to be MUST, though I also think this might be clearer to make two different statements for the two different cases, and make it clearer how this applies when you're subscribing to multiple paths, some of which exist and some of which don't. For example, maybe:

When a GET or POLL subscription includes paths that do not (yet) exist in the data tree, the target MUST NOT include those paths in its response (for example, a GET for exclusively paths that don't exist should return a SubscriptionResponse that does not contain any updates).

When a STREAM subscription includes paths that do not (yet) exist in the data tree, the target MUST continue to monitor for the existence of those paths, and transmit telemetry updates should any of them exist in the future. In particular, a STREAM subscription consisting solely of paths that are not in the current data tree MUST NOT close the RPC, but instead should continue monitoring in case those paths are created later on.

Copy link
Copy Markdown

@dan-lepage dan-lepage Sep 13, 2021

Choose a reason for hiding this comment

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

One question: By default, a STREAM starts by sending a notification of the current state; I assume in the case that none of the subscribed paths are set it should still send this notification, it just won't contain any updates?

Copy link
Copy Markdown
Contributor Author

@wenovus wenovus Sep 13, 2021

Choose a reason for hiding this comment

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

I'm happy with your modification, although I assume by GET you mean ONCE?

One question: By default, a STREAM starts by sending a notification of the current state; I assume in the case that none of the subscribed paths are set it should still send this notification, it just won't contain any updates?

This is also my understanding from https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#3523-sending-telemetry-updates

Copy link
Copy Markdown

@dan-lepage dan-lepage Sep 15, 2021

Choose a reason for hiding this comment

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

Oh sorry, yes, I meant ONCE.

Copy link
Copy Markdown
Member

@robshakir robshakir Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This LGTM, but one question here -- was the clarification that sync_response was not required something that was intended to be removed in the edit.

I can see a structure like:

For subscriptions that exclusively contain paths that do not (yet) exist in the data tree:

  • for ONCE
    • target sends no Notifications since there are no values set.
    • target sends sync_response
    • target closes RPC
  • for STREAM
    • Target sends no Notifications since there are no values set
    • target sends sync_response
    • target keeps RPC open and monitors for the paths - sending Notifications when they appear.

On the clarification w.r.t whether notifications are sent for unset paths in a STREAM subscription -- if a path isn't set a Notification isn't sent -- i.e., we don't expect to send a notification to say something was nil, so that there is consistent behaviour whether you're in this initial sync or not.

Copy link
Copy Markdown
Contributor Author

@wenovus wenovus Oct 21, 2021

Choose a reason for hiding this comment

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

Ok, updated to say that sync_response is expected as the first message, PTAL.

Copy link
Copy Markdown
Member

@dplore dplore Feb 10, 2022

Choose a reason for hiding this comment

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

@gcsl Can you give us your review?

NOT close the RPC for `STREAM` subscriptions, and instead should continue to
monitor for the existence of the path, and transmit telemetry updates should it
exist in the future.

<!-- TODO(robjs): Need a way to be able to send this information. Previously
said:
Expand Down