Conversation
| google.protobuf.Timestamp create_time = 5; | ||
| google.protobuf.Timestamp current_version_update_time = 6; | ||
| google.protobuf.Timestamp ramping_version_update_time = 7; | ||
| } |
| repeated WorkerDeploymentSummary worker_deployments = 2; | ||
|
|
||
| // (-- api-linter: core::0123::resource-annotation=disabled --) | ||
| message WorkerDeploymentSummary { |
There was a problem hiding this comment.
This looks like RoutingInfo, is it possible to reuse types? (if not RoutingInfo directly, have it be the first-and-for-now-only field in this message)
There was a problem hiding this comment.
My vote would be for something like this (also them WorkerDeploymentSummary has a subset of the fields in WorkerDeploymentInfo which is nice. You could even comment defining Summary as a subset of info and then there is a clear semantic about what should/should not be in each message.
There was a problem hiding this comment.
I agree that RoutingInfo should come here - my bad for not catching this - done!
carlydf
left a comment
carlydf
left a comment
There was a problem hiding this comment.
I agree with Chad about reusing RoutingInfo instead of repeating the fields, but other than that this looks good!
| repeated WorkerDeploymentSummary worker_deployments = 2; | ||
|
|
||
| // (-- api-linter: core::0123::resource-annotation=disabled --) | ||
| message WorkerDeploymentSummary { |
There was a problem hiding this comment.
My vote would be for something like this (also them WorkerDeploymentSummary has a subset of the fields in WorkerDeploymentInfo which is nice. You could even comment defining Summary as a subset of info and then there is a clear semantic about what should/should not be in each message.
| string name = 1; | ||
| string current_version = 2; | ||
| string ramping_version = 3; | ||
| float ramp_version_percentage = 4; | ||
| google.protobuf.Timestamp create_time = 5; | ||
| google.protobuf.Timestamp current_version_update_time = 6; | ||
| google.protobuf.Timestamp ramping_version_update_time = 7; |
There was a problem hiding this comment.
| string name = 1; | |
| string current_version = 2; | |
| string ramping_version = 3; | |
| float ramp_version_percentage = 4; | |
| google.protobuf.Timestamp create_time = 5; | |
| google.protobuf.Timestamp current_version_update_time = 6; | |
| google.protobuf.Timestamp ramping_version_update_time = 7; | |
| string name = 1; | |
| google.protobuf.Timestamp create_time = 2; | |
| RoutingInfo routing_info 3; |
|
LGTM |
4 participants
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
What changed?
Why?
Breaking changes
Server PR