Skip to content
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 FirstRunAt to StartWorkflowOptions #1360

Merged
merged 37 commits into from
Aug 1, 2024

Conversation

timl3136
Copy link
Contributor

@timl3136 timl3136 commented Jul 31, 2024

What changed?
Add FirstRunAt in StartWorkflowOptions. Server logic PR: uber/cadence#6178

Why?
Add the feature allowing client to specify a exact time for the first run of their workflow.

How did you test it?
Unit tests, also tested in cadence-samples repo as well as dev environment

Potential risks
It should not break anything.

Detailed Description
[In-depth description of the changes made to the interfaces, specifying new fields, removed fields, or modified data structures]
Add a new start workflow option called First_Run_At, which would affect the scheduled time for first run of a workflow.

Impact Analysis

  • Backward Compatibility: [Analysis of backward compatibility]

  • It’s an additional optional field so it should be compatible with all clients as the processing happens in the server

  • In terms of server backward compatibility, old server should be compatible with new requests as the field is added to the end of the request and will be dropped. The logic when this field is ignored will be the same as the older version.

  • Forward Compatibility: [Analysis of forward compatibility]

  • It’s forward compatible because new server will treat the value as unset when the old request that's missing that field is received.

Testing Plan

  • Unit Tests: [Do we have unit test covering the change?]
  • Yes, it would be implemented in both server and client repo
  • Persistence Tests: [If the change is related to a data type which is persisted, do we have persistence tests covering the change?]
  • The data will not be persisted
  • Integration Tests: [Do we have integration test covering the change?]
  • Compatibility Tests: [Have we done tests to test the backward and forward compatibility?]

Rollout Plan

  • What is the rollout plan?
  1. Add and merge the fields in cadence-idl repo
  2. Update SHA for cadence-idl repo and handled data fields in shared.go in cadence-server repo
  3. Rollout to all clusters before updating SHA on cadence-client
  4. Updating SHA for cadence-idl and add the fields in cadence-client
  5. Update client version in cadence-canary for monitoring
  6. Add canary workflows to ensure the new field work as expected
  7. Release new version in cadence-client with the added field
  • Does the order of deployment matter?
    Yes, we need to get the server build updated before deploying such change in client.
  • Is it safe to rollback? Does the order of rollback matter?
    Yes, rollback server version is safe given that the change is backward compatible.
  • Is there a kill switch to mitigate the impact immediately?
    No

Copy link

codecov bot commented Jul 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.43%. Comparing base (45557d5) to head (625ac21).
Report is 1 commits behind head on master.

Additional details and impacted files
Files Coverage Δ
internal/client.go 74.25% <ø> (ø)
internal/internal_workflow_client.go 90.48% <100.00%> (+0.16%) ⬆️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3ed20f1...625ac21. Read the comment docs.

@timl3136 timl3136 merged commit 7ba675b into uber-go:master Aug 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants