Skip to content

Conversation

@bergundy
Copy link
Member

What was changed:

Use the async core interface introduced in temporalio/sdk-core#77

Worker is now created with the static async Worker.create method.

How has this been tested?

Ran the existing tests which needed some modifications.

Any docs updates needed?

Docs were updated.

@bergundy bergundy requested a review from Sushisource March 30, 2021 13:13
@bergundy
Copy link
Member Author

Blocked on temporalio/sdk-core#80

/// Send an error to JS via callback using an [EventQueue]
fn send_error<T>(queue: Arc<EventQueue>, callback: Root<JsFunction>, error: T)
where
T: ::std::fmt::Display + Send + 'static,
Copy link
Member

Choose a reason for hiding this comment

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

You don't need the leading :: for these.

Copy link
Member Author

Choose a reason for hiding this comment

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

Habit I picked up, I don't mind getting rid of it.

};
let queue = Arc::new(cx.queue());

std::thread::spawn(move || {
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to break up this big closure into smaller functions

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, I'll do that

Comment on lines 164 to 166
if matches!(request, Request::BreakPoller) {
break;
} else if matches!(request, Request::Shutdown) {
Copy link
Member

Choose a reason for hiding this comment

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

This reads a bit strange compared to just a match expression

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you'll like the new structure

Copy link
Member

Choose a reason for hiding this comment

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

Yup, very nice. 🙌

Comment on lines 276 to 278
// Ignore BreakPoller and Shutdown, they're handled above
Request::BreakPoller => {}
Request::Shutdown => {}
Copy link
Member

Choose a reason for hiding this comment

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

Can be Request::BreakPoller | Request::Shutdown

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Comment on lines +312 to +314
let worker = cx.argument::<BoxedWorker>(0)?;
let queue_name = cx.argument::<JsString>(1)?.value(&mut cx);
let callback = cx.argument::<JsFunction>(2)?;
Copy link
Member

Choose a reason for hiding this comment

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

You could impl some kind of unpack function on FunctionContext that would de-dupe these preambles and make things a bit more readable

Copy link
Member Author

Choose a reason for hiding this comment

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

You'd have to show me what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

@bergundy Something like this

trait FnContextExt {
  fn unpack(self) -> (BoxedWorkerType, QueueNameType, CallbackType);
}

impl FnContextExt for FunctionContext {
    /// Implement the three lines here and return 3-tuple (or a real struct)
}

Copy link
Member Author

@bergundy bergundy left a comment

Choose a reason for hiding this comment

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

Thanks

/// Send an error to JS via callback using an [EventQueue]
fn send_error<T>(queue: Arc<EventQueue>, callback: Root<JsFunction>, error: T)
where
T: ::std::fmt::Display + Send + 'static,
Copy link
Member Author

Choose a reason for hiding this comment

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

Habit I picked up, I don't mind getting rid of it.

};
let queue = Arc::new(cx.queue());

std::thread::spawn(move || {
Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, I'll do that

Comment on lines 164 to 166
if matches!(request, Request::BreakPoller) {
break;
} else if matches!(request, Request::Shutdown) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I think you'll like the new structure

Comment on lines 276 to 278
// Ignore BreakPoller and Shutdown, they're handled above
Request::BreakPoller => {}
Request::Shutdown => {}
Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Comment on lines +312 to +314
let worker = cx.argument::<BoxedWorker>(0)?;
let queue_name = cx.argument::<JsString>(1)?.value(&mut cx);
let callback = cx.argument::<JsFunction>(2)?;
Copy link
Member Author

Choose a reason for hiding this comment

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

You'd have to show me what you mean.

@bergundy bergundy merged commit 7fb9e18 into main Mar 31, 2021
@bergundy bergundy deleted the async-core branch March 31, 2021 12:21
bergundy added a commit that referenced this pull request Apr 1, 2022
Implements the Worker part of [proposal #56](https://github.com/temporalio/proposals/blob/050825aba0e2e6cde91bae81945dce082bd47622/typescript/connections.md)
- Break down `Core` into `Runtime` and `NativeConnection`
- Workers now require a `NativeConnection` instance instead of using the singleton `Core` connection
- `Runtime` may be removed in the future but remains in the SDK to enable log forwarding from Core
- By default we don't forward Core logs into node anymore

This PR uses `sdk-core/master` which unblocks local activities and upsert search attributes, the last missing pieces for a feature complete SDK (apart from stack trace query).
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.

3 participants