Context RequestValues for ygnmi queries #139
Conversation
wenovus
commented
Dec 2, 2023
wenovus
commented
* Values are added at Subscribe/Get calls. * Tests added for both compress/uncompressed queries. NewContext/FromContext are the calls that add/extract context values.
Pull Request Test Coverage Report for Build 7073866121
💛 - Coveralls |
|
|
||
| // RequestValues contains request-scoped values for ygnmi queries. | ||
| type RequestValues struct { | ||
| // CompressedConfigQuery is a key type that means that the query is |
There was a problem hiding this comment.
It looks like q.IsState() now becomes two boolean. This added some impossible combinations that the consumer of this struct will have to deal with.
Why not just do IsState bool here?
There was a problem hiding this comment.
I thought about it, but I thought it would be more direct to have these fields and have the clear, directly comments rather than requiring any observers to know about the path filtering properties. It's impossible for the conflicting situation to occur when done correctly. WDYT?
There was a problem hiding this comment.
Previously I thought CompressedConfigQuery == false && CompressedStateQuery == false is an impossible condition as well as both true, but #140 clarified that both could be false when the query is not compressed. I don't think I fully understand why it matters if it's a compressed query.
Another way to design this with less ambiguity is to make it a call to action, i.e. WantConfig and WantState. It focuses on telling what the caller should do rather than relying on their judgment on what the signals meant. I guess if the query is not compressed, then we do want to keep both config and state in the result?
There was a problem hiding this comment.
ygot-generated code comes in two forms: compressed and uncompressed. When compressed it can only store either config or state, but not both. That's why it's possible for both CompressedConfigQuery and CompressedStateQuery to be false.
I agree a better name might be ConfigFiltered, and StateFiltered. This is the most direct call-to-action.
I've updated #140 to address this as well.
