Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Deployment Pre-Release APIs #479
Add Deployment Pre-Release APIs #479
Changes from 2 commits
aad202a
12ec5b2
bf6ec80
a432c29
d3bd873
6421777
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment doesn't add much information. Should define "deployment"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more information for both Deployment and DeploymentInfo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add last_poller_time if technically possible. Can be updated lazily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 That's possible and I agree we should have it. Will create a task for it (can't do right away because of more technical work).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it belongs here. As this time is per build id. Or this message should be renamed to the DeploymentTaskQueueInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is already nested in
DeploymentInfo
so I thought it's less verbose like this. But please lmk if you think it should still be renamed toDeploymentTaskQueueInfo
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message names are top level. So someone looking at the name will not see where it is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfateev This message name is nested, not top level. Are suggesting we should not have nested message definitions and only top level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this? is UNSPECIFIED not enough? Clarify in the comments...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider a future when we are able to use a different current deployment for different workflow types or task queues? To me it looks like we need a separate Table/Message/API to get all Start/ContinueAsNew records that point to deployments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've thought about some of these cases:
rollout
API. (i.e. instead of a single % ramp, we could provide various ramps for different types. Shahab mentioned there could be multi-cursor problems to think about with such, though). In that world, we could keep the high-level status ofDEPLOYMENT_STATUS_RAMPING
and create a more fine-grained API for power users. So, it seems premature to think too deeply now.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding to @drewhoskins-temporal comments, basically we envision eventually adding task queue and workflow type granularity to Rollout so user can ramp them one-by-one. However, setting different current deployment for different workflow types or task queues is not currently on the road map and users should create new deployment series for such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we clarify the ordering of the results in a comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be roughly sorted based on deployment creation time, decending. Although if a deployment has a very high number of TQs (or very high number of events happening to it) it will CaN and the sort will be mixed up. In reality we are relying on Visibility sort which is based on WF start time.
I prefer to keep this unspecified until we have a guaranteed order in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something's not right here. I don't see a use case for listing all the deployments, intermixed, regardless of deployment name. The two scenarios I can think of are users asking:
a) what are the deployment series? (or perhaps the current deployment for each series)
b) what's the history of a given deployment series?
If users do want them all in one endpoint, or if we want that for our UI, it would be a list of lists. But that seems puntable for the unblock , here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also question the value of an undisclosed, flaky ordering. Users will naturally expect a time-reverse ordering regardless of what we say here.
I would say a total ordering should be a hard requirement.
Is the scenario you mentioned very likely? Is it sufficiently unlikely to ship for the pre-release and then task up the improvement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the shuffle is sufficiently unlikely and we can fix it after pre-release. It also won't be a total shuffle, one/few over active deployment will be at the top, the fact that they are over active might mean they are also new builds.
Regarding list of lists, I agree such a UI will make sense and likely preferable. This is something I actually tried to bring up and get attention to but was not successful. Hopefully, it will be discussed during UI work and I'll be OK changing the list APIs then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why redefine
DeploymentInfo
without metadata and task_queue_infos here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the list is smaller? is the idea that if the caller wants more detailed info they would subsequently call
DescribeDeployment
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we don't want to make the list API expensive. For example, TQ info will not be immediately accessible from Visibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call it something different to avoid confusion, like DeploymentSummary or DeploymentBriefs or something like that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also seems odd to me. Why do users care if this API is expensive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drewhoskins-temporal are you suggesting the list should return complete info of all deployments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change the name to
DeploymentListEntry
orDeploymentSummary
. Same pattern is used for ListSchedules where we return a abbreviated set of info in the list response:api/temporal/api/schedule/v1/message.proto
Line 372 in a3ed90f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it to DeploymentListInfo similar to schedules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this API should exist as all changes to a deployment should happen through a rollout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mfateev I've been meaning to mention to you--for pre-release, we're planning to ship this lower-level API. We're working on the design for the rollout API for public preview, at which time we will support ramp pre-deploy testing as part of a bigger rollout API. Then, we can deprecate this API.
The team is currently focused on execution, but expect to review the rollout API design in December.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I annotated this and other APIs as "Experimental" in
service.proto
so it's clear that they all can change or be removed in the next release.As @drewhoskins-temporal said, this one in particular is merely a low-level placeholder until the rollout API arrives before public preview.