Scaffolding and bare-bones client #155
Conversation
| "start_workflow_execution" => { | ||
| rpc_call!(self, block, call, start_workflow_execution) | ||
| } | ||
| _ => Err(error!("Unknown RPC call {}", call.rpc)), |
There was a problem hiding this comment.
Obviously going to have a lot more here. We manually add these in .NET and Python because they don't get added that often, and we may do the same here, but we will have a test to confirm every one is present and works.
| * [Protobuf Compiler](https://protobuf.dev/) (i.e. `protoc` on the `PATH`) | ||
| * This repository, cloned recursively | ||
| * Change to the `temporalio/` directory |
There was a problem hiding this comment.
We chose to move this to a subdir because we expect other gems (e.g. temporalio-opentelemetry) and we don't want too many top-level files that move the README contents below the fold when arriving at the repo.
temporalio/Cargo.toml
Outdated
There was a problem hiding this comment.
There's a lot of things in this file that I blindly brought over from https://github.com/temporalio/sdk-core/blob/master/Cargo.toml. This is likely only going to every be a 1-member workspace (it's just needed at the top level for tooling). Consider getting rid of everything in here except members and moving things to specific project toml if they need to be retained.
There was a problem hiding this comment.
Added docs to the top of this file explaining why it has to be this way right now
temporalio/ext/src/runtime.rs
Outdated
| // Ignore fail since we don't properly catch panics in | ||
| // without_gvl right now | ||
| let _ = self.handle.async_command_tx.send(AsyncCommand::Shutdown); |
There was a problem hiding this comment.
Maybe error log here at least so if something weird is happening you might have an idea why
| # client. To create another client on the same connection, like for a different namespace, {dup_options} may be | ||
| # used to get the options as a struct which can then be altered and splatted as kwargs to the constructor (e.g. | ||
| # +Client.new(**my_options.to_h)+). |
There was a problem hiding this comment.
Not urgent but feels like it'd be a lot more convenient to provide a client.new_client_with_changes(...) function that you can just pass the kwargs of what you want to override and then it does this bit for you.
There was a problem hiding this comment.
This is a rare need I think, and I want to give people the ability to get their options back anyways. They may want to do more w/ them than just mutate+reinstantiate. What some Ruby libraries do is allow the configuration to occur in a block, e.g.:
my_new_client = my_client.new_with_changes do |options|
# What if I want to do more than just simple override?
options.namespace = 'my other namespace'
endBut I think this is a rare enough need (here and in other SDKs), that this is enough:
options = my_client.dup_options
options.namespace = 'my other namespace'
my_new_client = Client::new(**options.to_h)There was a problem hiding this comment.
Yeah, it's definitely not important. The former looks pretty nice I think but, I agree this is rare to need to do.
| desc.each do |method| | ||
| # Camel case to snake case | ||
| rpc = method.name.gsub(/([A-Z])/, '_\1').downcase.delete_prefix('_') | ||
| define_method(rpc.to_sym) do |request, rpc_retry: false, rpc_metadata: {}, rpc_timeout_ms: 0| |
There was a problem hiding this comment.
A little bit easier than all the macros I had to write lol
| rpc_timeout_ms: | ||
| ) | ||
| response_bytes = Bridge.async_call do |queue| | ||
| async_invoke_rpc( |
| # @return [LoggingOptions, nil] Logging options, default is new {LoggingOptions} with no parameters. | ||
| # @!attribute metrics | ||
| # @return [MetricsOptions, nil] Metrics options. | ||
| TelemetryOptions = Struct.new( |
| // TODO(cretz): Any reasonable way to prevent byte copy that is just going to get decoded into proto | ||
| // object? |
There was a problem hiding this comment.
There probably is, but it might also be more effort than it's worth. You'd have to pass around some explicit lifetime-having struct here or something. Hard for me to say without playing with the code myself but I could try it out if you like
There was a problem hiding this comment.
Yeah, I think I found a way with rb_str_new_static (which magnus doesn't expose directly, but uses for static literals in r_string! IIUC). To do it safely, I'd have to probably unhook it from Rust (i.e. in a forgotten box) and put it back for dropping on RString GC. Can't even have a predictable time it could be dropped because I think Ruby GC has certain expectations here even after use.
But yeah it's a dangerous lifetime game to play save these cycles. I'll consider it premature optimization for now and we can come back later if needed.
What was changed
Temporalio::Clientwith most basic features and a single basic testMany things were not done that will be done shortly including:
Ideally the review can focus more on the high-level approach and not get too bogged down in English of the API docs (which mostly came from Python and can be improved incrementally). Much of this is covered in the proposal.
Checklist