Make internal execution info consistent with public API #6860
Conversation
ShahabT
changed the title
| // completes and the workflow's deployment (or versioning override) is updated. | ||
| DeploymentTransition deployment_transition = 4; | ||
|
|
||
| message DeploymentTransition { |
There was a problem hiding this comment.
Just to double check: all these embedded messages are not use outside of persistence layer, right?
There was a problem hiding this comment.
yeah but they were also copied in the public API. Now I made the public ones flat and reusing them in here.
| // GetEffectiveVersioningBehavior returns the effective versioning behavior. | ||
| // VersioningOverride.Behavior, if set, takes precedence over Behavior. | ||
| GetEffectiveVersioningBehavior() enumspb.VersioningBehavior | ||
| GetOngoingDeploymentTransition() *persistencespb.WorkflowExecutionInfo_VersioningInfo_DeploymentTransition |
There was a problem hiding this comment.
Previously I tried very hard to avoid types like this and never used nested messages. I don't have strong opinion on it now, but this long path doesn't look nice.
There was a problem hiding this comment.
Transition is always ongoing. I think you can remove word "Ongoing" without loosing the meaning.
There was a problem hiding this comment.
agree with this. also, having both GetDeploymentTransition and GetOngoingDeploymentTransition makes the reader wonder "is there a difference between the two" when there is really not?
There was a problem hiding this comment.
Made the messages flat.
| // DeploymentTransition.Deployment takes precedence over VersioningOverride.Deployment, | ||
| // over Deployment. VersioningOverride.Deployment is only considered if the override | ||
| // behavior is PINNED. |
There was a problem hiding this comment.
As a person who have less context, I don't understand what is precedence what. I can guess where DeploymentTransition.Deployment is coming from (because there are methods below), but other two don't clarify anything. Just saying.
There was a problem hiding this comment.
improved the comments
| @@ -3165,7 +3165,7 @@ func (wh *WorkflowHandler) GetCurrentDeployment(ctx context.Context, request *wo | |||
|
|
|||
| describeDeploymentResponse, err := wh.deploymentStoreClient.GetCurrentDeployment(ctx, namespaceEntry, request.SeriesName) | |||
There was a problem hiding this comment.
should this function be called GetEffectiveDeployment?
There was a problem hiding this comment.
No this does not have to do with workflow execution, it's for current deployment within a deployment series.
## What changed? <!-- Describe what has changed in this PR --> Update internal MS versioning info to be consistent with what is defined in [the public API](https://github.com/temporalio/api/pull/487/files). ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
## What changed? <!-- Describe what has changed in this PR --> Update internal MS versioning info to be consistent with what is defined in [the public API](https://github.com/temporalio/api/pull/487/files). ## Why? <!-- Tell your future self why have you made these changes --> ## How did you test it? <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> ## Potential risks <!-- Assuming the worst case, what can be broken when deploying this change to production? --> ## Documentation <!-- Have you made sure this change doesn't falsify anything currently stated in `docs/`? If significant new behavior is added, have you described that in `docs/`? --> ## Is hotfix candidate? <!-- Is this PR a hotfix candidate or does it require a notification to be sent to the broader community? (Yes/No) -->
What changed?
Update internal MS versioning info to be consistent with what is defined in the public API.
Why?
How did you test it?
Potential risks
Documentation
Is hotfix candidate?