search

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

Make clear that `sync_response` is used for all three modes #156

Closed wenovus closed 2 years ago

robshakir commented 2 years ago

I think this makes sense -- is it currently implemented this way in the reference implementation? The reason I ask is that I can see some argument that since the RPC closes for ONCE, it is possible to glean this without needing sync_response. I think the argument for including it is that you then know the target was actually done with sending the data, rather than died mid-stream.

wenovus commented 2 years ago

Yes the reference implementation always sends a sync for ONCE: code: https://github.com/openconfig/gnmi/blob/39aa74195f0d7daf817dfdf26b8d976fd05e1cb9/subscribe/subscribe.go#L350 test: https://github.com/openconfig/gnmi/blob/39aa74195f0d7daf817dfdf26b8d976fd05e1cb9/subscribe/subscribe_test.go#L173-L175

Given EOF is not an error condition, but occurs naturally due to the server finishing its processing (ref), I agree for ONCE it's redundant. However, I can see for the sake of consistency that we keep the sync_response.

wenovus commented 2 years ago

Actually I see here https://github.com/openconfig/reference/blob/master/rpc/gnmi/gnmi-specification.md#35151-once-subscriptions

Following the transmission of all updates which correspond to data items within the set of paths specified within the subscription list, a SubscribeResponse message with the sync_response field set to true MUST be transmitted, and the RPC via which the SubscribeRequest was received MUST be closed.

So it seems to me proper to merge this unless we want to change the spec.

wenovus commented 2 years ago

Thanks for the review