Describe task queue #656
Describe task queue #656
Conversation
| rpc_metadata: Mapping[str, str] = {}, | ||
| rpc_timeout: Optional[timedelta] = None, | ||
| ) -> TaskQueueDescription: | ||
| """Describe task queue. |
There was a problem hiding this comment.
nit: This comment is redundant. Can you give a flavor for the type of information I should expect as a response and why I would use each one?
At this point I assumed I would get some info if I check neither and was wondering what it was. Wasn't until I looked at the code that I realized it wouldn't work.
There was a problem hiding this comment.
👍 I will clarify that this returns poller and stats information and that currently you need to provide at least one of the booleans.
| report_pollers: Include list of pollers for requested task queue types. | ||
| report_stats: Include task queue stats for requested task queue types. |
There was a problem hiding this comment.
Are these toggles here because it could degrade server performance to check them? Is there any kind of throughput limitation people should be aware of? (Otherwise, why are they necessary?)
There was a problem hiding this comment.
This is how the API was designed server side, but I would guess that they made these opt-in for performance reasons, yes. This is consistent with the Go SDK API that exists today already.
| if not report_pollers and not report_stats: | ||
| raise ValueError( | ||
| "At least one of report_pollers or report_stats must be True" | ||
| ) |
There was a problem hiding this comment.
nit: Oh I see, so one is required. I assumed not since they were both optional, and there was a default set of info.
If you want, you could avoid this friction, and runtime errors, by having a required enum field for either/both.
There was a problem hiding this comment.
An enum is a bit rough here because there are more coming (e.g. "report reachability"). We designed this as simple booleans to match Go SDK.
| @@ -1108,6 +1108,46 @@ async def get_worker_task_reachability( | |||
| ) | |||
| ) | |||
|
|
|||
| async def describe_task_queue( | |||
There was a problem hiding this comment.
Have we considered a top level scaling/tuning API instead that takes task queue types as a filter? That might be more discoverable for somebody perusing the CLI/API command surface, who knows they want to do scaling but doesn't think of that when they see task queue.
It would also allow us to add deploy group as a filter once that ships.
And finally, it could allow us to embed other information within the API call such as related namespace-level information, for example if the namespace has rate limits that should also be taken into account.
There was a problem hiding this comment.
Additional context. Versioning team originally shipped APIs within task queue, and Max asked us to make them top-level and simply refer to task queue from them. (Later on, we decided to switch to deploy group, but the top-level API concept remains.)
This feels like it could be analogous.
There was a problem hiding this comment.
Have we considered a top level scaling/tuning API instead that takes task queue types as a filter?
I am not sure. This is the Python review for the SDK API that already exists in Go and calls the already existing server API. I think existential questions concerning the API may need to be asked in a place with broader reach.
|
|
||
|
|
||
| @dataclass | ||
| class TaskQueueDescription: |
There was a problem hiding this comment.
How this struct will look like once the per-buildId info is added?
There was a problem hiding this comment.
We don't know, so we are trying not to guess. We are trying to build this as if versioning does not exist from a user POV (because it doesn't).
| class TaskQueueDescription: | ||
| """Description of a task queue.""" | ||
|
|
||
| types: Mapping[TaskQueueType, TaskQueueTypeInfo] |
There was a problem hiding this comment.
I'd prefer the following.
| types: Mapping[TaskQueueType, TaskQueueTypeInfo] | |
| unversioned_types: Mapping[TaskQueueType, TaskQueueTypeInfo] |
There was a problem hiding this comment.
My reasoning is as follows.
The following diagram shows different "levels of task queue". ["Task Queue" is a overloaded term and it's been used to refer to any of the boxes in this diagram! In the server code at least we've refactored things and gave each box a different name.]
Now, from users perspective, Task Queue almost alway is the most top-level, black box. So DescribeTaskQueue, without any other specification, should describe that box.
Unversioned is just a slice of the whole Task Queue. A top-level field such as types is reasonable only if it maps to all slices (i.e. unversioned + build IDs). As server does not return all slices (and we rely on user specify the slices as of now), we cannot have a such top-level field. Maybe we can add it in the future.
As long as only unversioned stats are desired, it's totally fine, IMO, to put it in the top level proto but it should be qualified as such so user does not interpret it as something that holds the stats for all slices.
There was a problem hiding this comment.
I think people not using versioning should not have to know anything about versioning or have the term versioning in their code anywhere. Can we pretend these stats were made available before versioning was a thing (which is kinda the case since versioning is not a thing yet)?
There was a problem hiding this comment.
I think we should call things what they really are. We cannot pretend versioning is not there because it is. Even if it is pre-release, we know it's going to be public-preview and GA eventually.
If we call this only types it'll be dangerous or confusing for people who use versioning.
In your proposal as well you do have a note that talks about versioning and unversioned, so it's not like those terms will be completely absent from developers mind.
Lastly, I'm not sure if not having the term "unversioned" in the code anywhere for users not using versioning should be a goal TBH. I agree that they should not have to use term "version" or "build ID" in their code though.
There was a problem hiding this comment.
I think we should call things what they really are
I don't expect we're going to change Worker to UnversionedWorker either. We accept that the default is unversioned and we don't infect the default with the word unversioned anywhere. I do not think we should inconsistently start doing that.
In your proposal as well you do have a note that talks about versioning and unversioned, so it's not like those terms will be completely absent from developers mind.
I don't propose removing documentation about versioning, just API terms for those not using versioning.
If we call this only types it'll be dangerous or confusing for people who use versioning.
I think we can document this away. Same as anywhere else where unversioned default differs for people using versioning.
Versioning does not meaningfully exist right now, I don't think we should code as if it does. We're only punishing people with this terminology because we waited to implement backlog stats until some versioning parts were put into the API.
What was changed
Implementation of describe task queue as described at temporalio/features#525.
This is currently just a draft since it was opened early to get feedback.