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 activity context for rust sdk #302

Merged
merged 8 commits into from Apr 21, 2022

Conversation

dt665m
Copy link
Contributor

@dt665m dt665m commented Apr 16, 2022

What was changed

An activity context that tries to be as close to the go sdk as possible. Implementation added in sdk/src/activity_context.rs. sdk/src/lib.rs was also modified to support the new context for each activity worker function.

  1. Closes [Feature Request] Rust SDK Activity Context #301

  2. How was this tested:

  • Ran Unit Tests
  • Ran Integration tests
  • Sanity Tested with External Project on Docker-compose temporal environment
  1. Any docs updates needed?
    Not that I know of. Would be happy to contribute with some direction.

@CLAassistant
Copy link

CLAassistant commented Apr 16, 2022

CLA assistant check
All committers have signed the CLA.

@dt665m
Copy link
Contributor Author

dt665m commented Apr 16, 2022

@Sushisource Hope this could serve as a starting point for activities in the rust SDK.

@Sushisource
Copy link
Member

@dt665m Cool! Thanks for the PR.

One thing I haven't decided yet is if we should expose the context as an explicit parameter as is done here, or if we want to make it implicit and accessed via static functions (which is typical of all the non-Go SDKs). I would do the same thing with the workflow context if so.

Any thoughts on that?

That aside I think this is a good starting point.

Comment on lines 111 to 112
/// #NOTE the first parameter is popped for convenience, the rest is still stored in the
/// activity state. There are no variadic parameters in Rust.
Copy link
Member

Choose a reason for hiding this comment

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

This is probably a bit confusing. I think I want to continue enforcing that Rust-defined activities/workflows only take 1 parameter.

In that case, we should maybe give it a more specific name like extra_inputs . The docstring can indicate the first parameter is passed to the activity function explicitly, and there should not be any extra unless the caller is doing something a bit odd by passing more than one, and those can be retrieved here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by renaming + docs. I think it's healthy to keep the function even if a little confusing because it is possible to have polyglot environments where workflows are deployed from Go, but certain activities are handled in Rust specific task_queues. In that case, it could be surprising if the Go SDK naturally allows many parameters but Rust's activities can never access them.

@dt665m
Copy link
Contributor Author

dt665m commented Apr 18, 2022

@dt665m Cool! Thanks for the PR.

One thing I haven't decided yet is if we should expose the context as an explicit parameter as is done here, or if we want to make it implicit and accessed via static functions (which is typical of all the non-Go SDKs). I would do the same thing with the workflow context if so.

Any thoughts on that?

That aside I think this is a good starting point.

I don't have strong feelings either way. Intuitively, it seems like the static function style would make the default workflow/activities easier to pick up and learn. However, I imagine as soon as you actually start doing interesting workflows, you'll inevitably need the context.

Are you thinking of implementing this through tokio's task_local LocalKey?

@Sushisource
Copy link
Member

Sushisource commented Apr 18, 2022

I imagine as soon as you actually start doing interesting workflows, you'll inevitably need the context.

Can you elaborate a bit on any examples you're thinking of? Would be curious to hear what that is. Java and TS sdks don't have explicit context parameters for example.

Are you thinking of implementing this through tokio's task_local LocalKey?

Yeah. There actually is already one here:

static ACT_CANCEL_TOK: CancellationToken
which we should eliminate if we got with the explicit parameter.

@dt665m
Copy link
Contributor Author

dt665m commented Apr 19, 2022

I imagine as soon as you actually start doing interesting workflows, you'll inevitably need the context.

Can you elaborate a bit on any examples you're thinking of? Would be curious to hear what that is. Java and TS sdks don't have explicit context parameters for example.

Are you thinking of implementing this through tokio's task_local LocalKey?

Yeah. There actually is already one here:

static ACT_CANCEL_TOK: CancellationToken

which we should eliminate if we got with the explicit parameter.

If you decide to go with the task_local, we can fold the cancellation token into the struct. I think it really depends on the 90/10 use case.

To elaborate a bit, I want to be able to use snapshots and heartbeat details to handle idempotent requests (ie. commit some key/id first before making some API calls). I haven't done this yet... but I plan on trying long living workflows with asynchronous activity completion as well. Additionally, I want to tag the workflow id in activities as a tracing ray. These things all require the context. Doing task_local or explicit parameter is just semantically different. Both ways achieve the same things. As my use cases mostly require the context, it seems natural to pass one to the closures. Again, I don't feel strongly either way and if you decide to go with the static approach for API parity, I can make that change.

I can't see what is erroring in the buildkite though, so I'll need some help/direction on what is failing. I've run the buildkite integration test pipeline locally using the docker-compose files and only got warnings that I'm unsure on how to fix.

@Sushisource
Copy link
Member

I think let's go with explicit for now then. I think it's probably more idiomatic for Rust, and it's certainly more obvious what's going on.

Re: Buildkite failures - we'll probably move this repo to GHA soon, but for now it's just failing on lints and formatting. cargo fmt will fix the formatting and cargo clippy --test integ_tests --test load_tests --all && cargo clippy --all --all-features --all-targets will cover all lints.

@dt665m
Copy link
Contributor Author

dt665m commented Apr 20, 2022

I think let's go with explicit for now then. I think it's probably more idiomatic for Rust, and it's certainly more obvious what's going on.

Re: Buildkite failures - we'll probably move this repo to GHA soon, but for now it's just failing on lints and formatting. cargo fmt will fix the formatting and cargo clippy --test integ_tests --test load_tests --all && cargo clippy --all --all-features --all-targets will cover all lints.

Alright, explicit param it is. I'll try to run the clippy stuff to find things to fix. Should we also move the cancellation token into the Activity Context?

@Sushisource
Copy link
Member

Yeah, I think it should be moved in. Thanks!

@dt665m
Copy link
Contributor Author

dt665m commented Apr 21, 2022

Yeah, I think it should be moved in. Thanks!

I think I got most of the stuff we discussed covered. Buildkite is passing now as well. Hope this is good enough for a merge back to upstream. Thanks for the guidance! If I get some free time, I'm gonna need more for implementing workflow checkpoints. If that works out, the Rust SDK will have enough for me to not have to maintain another go project for workflows.

Copy link
Member

@Sushisource Sushisource left a comment

Choose a reason for hiding this comment

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

A few more things I'd like to fix but hitting approve so you don't need to wait on another round for me. If you want fix them, awesome, if not I'll address them. (Though I'm realizing I probably have to hit merge anyway, so just let me know and if you'd prefer me to make them I'll merge this first)

Made things easy and just pushed 'em myself

Thanks so much for this awesome contribution!

sdk/src/activity_context.rs Outdated Show resolved Hide resolved
sdk/src/activity_context.rs Outdated Show resolved Hide resolved
sdk/src/activity_context.rs Outdated Show resolved Hide resolved
sdk/src/activity_context.rs Outdated Show resolved Hide resolved
sdk/src/lib.rs Outdated Show resolved Hide resolved
@Sushisource Sushisource enabled auto-merge (squash) April 21, 2022 21:27
@Sushisource Sushisource merged commit f1ddc95 into temporalio:master Apr 21, 2022
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.

[Feature Request] Rust SDK Activity Context
3 participants