openconfig / reference

This repository contains reference implementations, specifications and tooling related to OpenConfig-based network management.
Apache License 2.0
155 stars 88 forks source link

multiple Notifications in SubscribeResponse (proto vs spec) #59

Closed jsterne closed 2 years ago

jsterne commented 7 years ago

Section 3.5.2.1 "Bundling of Telemetry Updates" says the following: "Since multiple Notification messages can be included in the update field of a SubscribeResponse message..."

But the gnmi.proto has this (Notification update is not a repeated field): message SubscribeResponse { oneof response { Notification update = 1; // Changed or sampled value for a path. bool sync_response = 3; // Indicate target has sent all values once. Error error = 4; // Report an occurred in the Subscription. } }

https://developers.google.com/protocol-buffers/docs/proto3 says the following about oneof: "You can add fields of any type, but cannot use repeated fields."

Is the spec incorrect or is the intention to support multiple Notifications in a single SubscribeResponse message ? (or am I misunderstanding something ?).

Note that a single Notification can have multiple Updates & Deletes (but there is only a single timestamp for the Notification as a whole).

Regards, Jason

robshakir commented 7 years ago

This is an error, yes, we need to rather say that multiple updates can be contained within the Notification.

message Notification {
  int64 timestamp = 1;          // Timestamp in nanoseconds since Epoch.
  Path prefix = 2;              // Prefix used for paths in the message.
  // An alias for the path specified in the prefix field.
  // Reference: gNMI Specification Section 2.4.2
  string alias = 3;
  repeated Update update = 4;   // Data elements that have changed values.
  repeated Path delete = 5;     // Data elements that have been deleted.
}

To send multiple updates you put the individual leaf updates into the update field of the single Notification that has a single timestamp field relevant to them all.

robshakir commented 2 years ago

This was addressed in a previous version of the specification. Thanks for raising it @jsterne.