alexeyinkin commented on code in PR #25610:
URL: https://github.com/apache/beam/pull/25610#discussion_r1128980772
##########
playground/api/v1/api.proto:
##########
@@ -314,6 +314,17 @@ message GetSnippetResponse {
Complexity complexity = 4;
}
+// GetMetadataRequest represents request for runner metadata
+message GetMetadataRequest {}
+
+// GetMetadataResponse contains metadata about the runner
+message GetMetadataResponse {
+ string runner_sdk = 1;
+ optional string build_commit_hash = 2;
+ optional int64 build_commit_timestamp_seconds_since_epoch = 3;
+ optional string beam_sdk_version = 4;
Review Comment:
From the same link
https://cloud.google.com/apis/design/design_patterns.md#optional_primitive_fields
> If not using optional would add complexity or ambiguity, then use optional
primitive fields.
This is why I asked for `optional` here. This endpoint exists in both router
and runners. The router is unaware of Beam and would have to return a magic
value instead if omitting it. I think that omitting it on the protocol level is
better.
There is indeed a bug in the current Dart implementation. It parses the raw
data correctly, note the `null` in the router's response for Beam version:
But then the getter returns an empty string for it.
The issue to make those getters nullable is the most thumbed-up one in the
repository, and members agree:
https://github.com/google/protobuf.dart/issues/523
So I believe it will be done at some point. So if the support is almost
there, I prefer to provision this right at the high level of the protocol to be
able to improve the implementation soon after.
I found no other explanations of discouraging `optional` so I vote for using
it.
There is another reason for going `optional`. On the frontend we have many
enums coming from the backend: SDK, Complexity, etc. In protobuf they come as
magic values like `SDK_UNDEFINED`. So we must maintain two versions of such
enum classes: one with this value to be parsed from the protofub, and the other
one is without it to be used in business logic. Converting between the two is
complexity, so in the next phase of Playground development we would like to
switch those to optional too. It would then be consistent with optional fields
in this endpoint.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
