-
Notifications
You must be signed in to change notification settings - Fork 91
Update gnmi-specification.md Target_Defined mode description #227
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
base: master
Are you sure you want to change the base?
Conversation
Add clarity on using sample-interval in target defined mode.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
ashu-ciena
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes appearing in this PR, in other than target defined mode section, are not done by me. In my copy of the fork, these changes are already fixed. So, I am not sure why these unrelated changes are appearing in this PR.
|
@dplore Can you please add relevant reviewers to this PR? Any information missing in the PR ? |
|
I've added this to the queue for OC Operator review and pinged a couple of individual reviewers. Making this mandatory would make it a breaking change to the base GNMI specification which we should not allow. I don't think we expect or require an sample_interval for TARGET_DEFINED. The target may choose it's own sample_interval. I can see this as possibly an optional field for TARGET_DEFINED subscriptions. |
Review comments incorporated.
|
Reviewed in July 29, 2025 OC Operators meeting. An additional idea is that this "if the target decides to use sample, the client suggests a sample interval" idea should be implemented in a new gnmi subscription mode. This way the target can communicate if the "suggested sample interval" feature is supported or not. I think this PR would benefit from a more information about the operational use cases we are trying to solve for before we can provide better feedback of what should be implemented. |
Are you suggesting a new subscription mode? I was wondering if the user subscribes in target-defined mode and also provides the sample interval, the target can then communicate if the "suggested sample interval" is viable or not on the subscribed xpath. If , there is no sample interval provided, target can choose. |
@dplore and @robshakir do you want more discussion on this in Thursday meeting? If not, can we give our final set of comments on the latest upload and we can proceed to merge? |
|
Following up on this thread, are there any open questions/comments? This PR requires approval, looking forward to it. |
Incorporated review comments in target-defined mode text change
|
I am note sure why this PR is in "Waiting For Author" state? I think it should be moved to "Ready to discuss" or "In Progress" mode. I have incorporated all the comments, I received so far. |
|
Moving to ready-to-discuss. The ability to specify an interval when setting the value of |
|
Reviewed in Oct 14, 2025 OC Operators meeting. General feedback is:
|
Suggestion for discussion: If |
|
@dplore and @earies the text in the current state of spec is only focusing on the type of subscription in target defined mode. So, we have the liberty to add clarifications regarding the sample interval, if target chose to treat the subscription as sample mode. Not specifying the sample interval is equivalent to specifying it as "0" value. The Subscription proto is same for both sample and target defined mode, so we can not restrict users to provide a sample interval even in target defined mode. I somewhat agree that if user does, target can choose to accept or reject the subscription with "Invalid Parameter" error. If you all agree, I can update my PR with these clarifications. |
|
The issue is the currently proposed text to be added for TARGET_DEFINED conflicts with the gnmi.proto field definition for It is ok to propose text that says use of |
|
If you are very interested in allowing a client to optionally choose a |
With TARGET_DEFINED, as the name suggests the "TARGET" should define the sample interval as well. Implementors may choose to use "default" sample interval in TARGET_DEFINED mode, just like when sample_interval is set to zero in SAMPLE mode. Thus no change to the gnmi notification message is required either. |
Clarify behavior of sample_interval in TARGET_DEFINED subscriptions.
|
I have updated the PR with the suggested text. Please do a final review. |
Add clarity on using sample-interval in target defined mode.