Add documentation for data_moniker.proto#177
Conversation
- Mark unused field as deprecated. - Add cc_enable_arenas option for C++.
| // order of data values returned matches the order of the read monikers. | ||
| // The semantics for when data is published and how updates are triggered | ||
| // is implementation dependent. | ||
| rpc StreamRead(MonikerList) returns (stream MonikerReadResult) {}; |
There was a problem hiding this comment.
NOTE: datastore-service implementation currently ignores all read monikers but the first one. It doesn't error it just ignores them.
| // positionally with the monikers declared in the first message. Implementations | ||
| // may choose to return a response message after each request has been processed | ||
| // for flow control purposes or no response message at all. | ||
| rpc StreamWrite(stream MonikerWriteRequest) returns (stream StreamWriteResponse) {}; |
There was a problem hiding this comment.
NOTE: datastore-service implementation sends response after each write request is processed, presumably for flow control? The grpc-device and grpc-sideband implementations never send any response messages.
| message MonikerList { | ||
| bool is_initial_write = 1; | ||
| // Deprecated. Preserved for backward compatibility. | ||
| bool is_initial_write = 1 [deprecated = true]; |
There was a problem hiding this comment.
NOTE: I marked this field as deprecated since it is not currently being used. I should say it is only used by grpc-sideband. However, it is used in a manner that is inconsistent with the naming of the field, and the implementation looks functionally incorrect. It appears to try and use this field to control whether write monikers are updated before reading from the read monikers, or whether read monikers are read from before write monikers are updated.
| // Communication continues in this ping-pong fashion until the stream is closed. | ||
| rpc ReadWriteValuesForMonikersStream(stream ReadWriteValuesForMonikersStreamRequest) returns (stream ReadWriteValuesForMonikersStreamResult) {}; | ||
|
|
||
| // This would be BatchReadValueFromMoniker following google AIPs |
There was a problem hiding this comment.
The following RPCs are included as possible API extensions that influenced the naming of the RPCs above. They would not immediately be part of the v2 API unless discussion here steers things in that direction.
- ReadValueFromMonikers - Read 1 value from N monikers
- ReadValuesFromMonikerStream - Read N values from a single moniker. This is the current implementation of
StreamReadfor thedatastore-service. - ReadChunkedValueFromMonikerStream - This would be a streaming API to read a single large data value from a single moniker. I haven't spent much energy thinking about the message definitions for this type of API.
| // Service for reading and writing data values using data monikers. | ||
| // Not all implementations implement all RPCs and clients should | ||
| // handle this gracefully. | ||
| service DataMonikerService { |
There was a problem hiding this comment.
NOTE: All of the sideband APIs have been removed from v2. I think it makes sense for it to be its own independent service.
| // Reads the current value for a single moniker. If no data value is available, | ||
| // Implementations may choose to return an empty response. | ||
| // Returns NOT_FOUND error if the moniker does not exist. | ||
| rpc ReadValueFromMoniker(ReadValueFromMonikerRequest) returns (ReadValueFromMonikerResponse) {}; |
There was a problem hiding this comment.
NOTE: I settled on a naming convention where:
- Value vs. Values is used to indicate the API returns a single value or multiple values for a single moniker.
- Moniker vs. Monikers is used to indicate reading/writing data to a single moniker or multiple monikers.
- Stream is used to denote a streaming API that has some handshake semantics to it. It is not simply a batch API where everything is sent in one big batch using a unary API.
| // data values returned matches the moniker order specified in the request | ||
| // message. The semantics for when data is published and how updates are | ||
| // triggered is implementation dependent. | ||
| rpc ReadValuesFromMonikersStream(ReadValuesFromMonikersStreamRequest) returns (stream ReadValuesFromMonikersStreamResponse) {}; |
There was a problem hiding this comment.
I considered naming this something like SubscribeToMonikerUpdates and modifying the response message so the data was self identifying in terms of which moniker it came from so values from different monikers could be returned at different rates. At that point it was tempting to make this a bidirectional stream where subscriptions could be added/removed dynamically. This seemed too much like a pub/sub API so I held off moving things in that direction.
bkeryan
left a comment
There was a problem hiding this comment.
Note: I reviewed only for style, not content
| // values in each write request must match the number of monikers supplied | ||
| // in the initialize request, and the data values are paired positionally with | ||
| // the monikers specified in the initialize message. | ||
| rpc WriteValuesToMonikersStream(stream WriteValuesToMonikersStreamRequest) returns (WriteValuesToMonikersStreamResponse) {}; |
There was a problem hiding this comment.
NOTE: This no longer returns a response stream. I didn't really see a purpose for acknowledging the writes with an empty response message so I removed it.
With that said, I can see the argument to keeping a response stream for better API extensibility in the future without breaking backward compatibility. So if others think it would be good to keep around for that purpose, I wouldn't fight it. Looking through the googleapis repo, any streaming APIs they have are almost always bidirectional streaming which I suspect is for this purpose.
| repeated google.protobuf.Any values = 1; | ||
| } | ||
|
|
||
| message WriteValuesToMonikersStreamRequest { |
There was a problem hiding this comment.
I borrowed this API pattern from a number of google streaming APIs (e.g. subscriber.proto). Each of the unique protocol or handshake messages are declared as nested messages. In this case, I declared the nested messages inline, but they could be moved top level if desired.
Most of the APIs I saw also create a corresponding response message which I didn't do here for simplicity. I didn't necessarily think there was value in making the client consume and throw away a response for the initialize phase of the stream and deal with the oneof from that point forward. With the said, adding a oneof now provides the best path forward for API extensibility without breaking backward compatibility as we can add new control/status messages to the stream protocol as the need arises.
| rpc StreamReadWrite(stream MonikerWriteRequest) returns (stream MonikerReadResult) {}; | ||
|
|
||
| // Reads the current value for a single moniker. If no data value is available, | ||
| // Implementations may choose to return an empty response or a NOT_FOUND error. |
There was a problem hiding this comment.
NOTE: datastore-service implementation returns empty response rather than an error if the moniker does not exist.
What does this Pull Request accomplish?
Updates
data_moniker.proto:cc_enable_arenasoption for C++.Adds a draft of a V2 version of the API that adheres to our protobuf guidelines and conventions and cleans the API up further.
Why should this Pull Request be merged?
I am posting this draft PR for further discussion. There are currently 5 forked copies of this proto file across various public repos:
grpc-deviceandnidaqmx-pythoncontain the exact same source but there are four unique variations in total, and none of them are API compatible with each other. The shape of the API is mostly the same but they have different package names, service names, and message names in some cases. We would like to gradually converge on a unified API that is sourced from this repo and consumed by the other repos. In that spirit, we would like to make small changes where appropriate to the current API that nudge things in that direction that don't break backward compatibility in a fashion that we are not comfortable with. I have also created a draft of a V2 API which can serve as a consolidated API for all repos and clean things up from the current API which do not conform to our conventions.In terms of service implementations, it appears there are currently three:
grpc-device,grpc-sideband, anddatastore-service. However, the implementation fromgrpc-sidebanddoesn't really appear to be used and might be dead code that is more of a reference implementation than anything?What testing has been done?
N/A