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

Updated state interfaces #1325

Merged
merged 19 commits into from
May 21, 2024
Merged

Updated state interfaces #1325

merged 19 commits into from
May 21, 2024

Conversation

tonyhb
Copy link
Contributor

@tonyhb tonyhb commented May 7, 2024

Type of change (choose one)

  • Chore (refactors, upgrades, etc.)
  • Bug fix (non-breaking change that fixes an issue)
  • Security fix (non-breaking change that fixes a potential vulnerability)
  • Docs
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)

Checklist

  • I've linked any associated issues to this PR.
  • I've tested my own changes.

Check our Pull Request Guidelines

Copy link

vercel bot commented May 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 0:36am

@tonyhb tonyhb force-pushed the feature/state-interfaces branch 2 times, most recently from 8af10bd to e71eb59 Compare May 7, 2024 01:35
Copy link
Collaborator

@darwin67 darwin67 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The extractTraceCtx need to be added back or it'll break trace propagation.
Also added some questions.

pkg/execution/executor/executor.go Show resolved Hide resolved
if err != nil {
// generate a new one here to be used for subsequent runs.
// this could happen for runs that started before this feature was introduced.
sid := telemetry.NewSpanID(ctx)
fnSpanID = &sid
// TODO: Save span ID // UPDATE?
Copy link
Collaborator

@darwin67 darwin67 May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should only save if metadata doesn't have a spanID already stored.
this is likely required if people do future scheduling with event.ts or just sleeps for a long time.
though the function run will look weird in that case since it'll look like the function started from the middle.

or we just don't care since it's no longer an issue once outside the history window.

pkg/execution/executor/executor.go Show resolved Hide resolved
pkg/execution/executor/executor.go Outdated Show resolved Hide resolved
pkg/execution/executor/executor.go Outdated Show resolved Hide resolved
pkg/execution/driver/driver.go Show resolved Hide resolved
pkg/execution/execution.go Show resolved Hide resolved
pkg/execution/executor/validate.go Show resolved Hide resolved
pkg/execution/executor/validate.go Show resolved Hide resolved
pkg/execution/state/v2/interfaces.go Show resolved Hide resolved
@tonyhb tonyhb force-pushed the feature/state-interfaces branch from e148c5c to df95d53 Compare May 9, 2024 22:59
pkg/execution/driver/driver.go Outdated Show resolved Hide resolved
pkg/execution/driver/driver.go Outdated Show resolved Hide resolved
BrunoScheufler and others added 2 commits May 17, 2024 13:14
We were missing tenant information in some calls to the v2 state interface. We'll be using the accountId for progressively rolling out the new interface.
@jpwilliams jpwilliams merged commit 930e431 into main May 21, 2024
13 checks passed
@jpwilliams jpwilliams deleted the feature/state-interfaces branch May 21, 2024 15:32
darwin67 added a commit that referenced this pull request May 22, 2024
Embed the event data into pauses so they can be referenced when needed for spans.

Also update some data reference due to changes from #1325

CronSchedule is removed from the data structure since it's not referenced anywhere, and is embedded into `Context` map instead.

---------
Co-authored-by: Darwin D Wu <[email protected]>
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.

None yet

4 participants