DescribeWorkerDeployment proto changes#523
Conversation
| // - It does not recieve new executions (see WorkerDeploymentVersionInfo.accepts_new_executions) | ||
| // - It has no active pollers (see WorkerDeploymentVersionInfo.pollers_status) | ||
| // - It is drained (see WorkerDeploymentVersionInfo.drainage_status) |
There was a problem hiding this comment.
fix indentation on these lines
| // - It does not recieve new executions (see WorkerDeploymentVersionInfo.accepts_new_executions) | ||
| // - It has no active pollers (see WorkerDeploymentVersionInfo.pollers_status) | ||
| // - It is drained (see WorkerDeploymentVersionInfo.drainage_status) |
There was a problem hiding this comment.
the fields referred to don't exist yet? I see a TODO.. is that going to come in this PR or later?
There was a problem hiding this comment.
yep - those fields will be coming in later when we start working on them
| } | ||
|
|
||
| message DescribeWorkerDeploymentResponse { | ||
| deployment.v1.WorkerDeploymentInfo deployment_info = 1; |
There was a problem hiding this comment.
We always use the fully qualified name when referring to other pacakges (e.g. temporal.api.deployment.v1...). I see some other deployment-related messages didn't do that but let's do it consistently going forward
| // Identity of the client that last edited the routing config for this Worker Deployment. | ||
| string last_editor_identity = 3; |
There was a problem hiding this comment.
If current/ramping version are added later, this field is better to be added with them because they change together.
| bool accepts_new_executions = 4; | ||
| } | ||
|
|
||
| // TODO (Shivam): WorkerDeploymentVersionSummary.PollerStatus + TaskQueueInfo + RampingInfo + |
There was a problem hiding this comment.
It seems appropriate to add create_time in the first iteration because no extra logic/data is needed to populate that.
|
|
||
| rpc DescribeWorkerDeployment (DescribeWorkerDeploymentRequest) returns (DescribeWorkerDeploymentResponse) { | ||
| option (google.api.http) = { | ||
| get: "/namespaces/{namespace}/worker-deployment/{deployment_name}/describe-worker-deployment" |
There was a problem hiding this comment.
No need for the /describe-worker-deployment part. Describe is usually a simple GET on the resource top level endpoint. See DescribeTaskQueue as an example.
| get: "/namespaces/{namespace}/worker-deployment/{deployment_name}/describe-worker-deployment" | |
| get: "/namespaces/{namespace}/worker-deployment/{deployment_name}" |
| get: "/namespaces/{namespace}/worker-deployment/{deployment_name}" | ||
| additional_bindings { | ||
| get: "/api/v1/namespaces/{namespace}/worker-deployment/{deployment_name}" | ||
| } |
There was a problem hiding this comment.
| get: "/namespaces/{namespace}/worker-deployment/{deployment_name}" | |
| additional_bindings { | |
| get: "/api/v1/namespaces/{namespace}/worker-deployment/{deployment_name}" | |
| } | |
| get: "/namespaces/{namespace}/worker-deployments/{deployment_name}" | |
| additional_bindings { | |
| get: "/api/v1/namespaces/{namespace}/worker-deployments/{deployment_name}" | |
| } |
Prefer plurals
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?
DescribeWorkerDeploymentWhy?
Breaking changes
Server PR