ruby sdk dev guide #3560
ruby sdk dev guide #3560
Conversation
…into ruby-dev-guide
…into ruby-dev-guide :wq
| ``` | ||
|
|
||
| There are several other details not covered here about futures, such as how exceptions are handled, how to use a setter | ||
| proc instead of a block, etc. See the API documentation for details. |
There was a problem hiding this comment.
This should be a link, probably?
There was a problem hiding this comment.
The "API documentation" is actually many anchor links depending on what they are trying to do. May be able to link to https://ruby.temporal.io/Temporalio/Workflow/Future.html as a container for those other things they can do.
| end | ||
| ``` | ||
|
|
||
| ### Workflow Futures {#workflow-futures} |
There was a problem hiding this comment.
This H3 should be moved to an H4 at the end of the "Start Activity Execution" section imo, explaining Futures before Activities is unintuitive.
There was a problem hiding this comment.
Not following. Futures have nothing to do with activities, they are a workflow utility for concurrent code running.
There was a problem hiding this comment.
Right -- but if someone is reading this page top to bottom for the first time and they've just encountered the concept of a Workflow (a real possibility, if they are a Ruby developer first approaching Temporal), Futures are not going to make sense to them as the very first Workflow feature they've learned about. They do make sense in the context of concurrently dispatching Activities, and I believe this is the context we typically bring them up in. If this isn't clean enough conceptually for you, I would strongly advocate for them to go on a different page, because I think we are reproducing a very common complaint about our docs otherwise, namely that they introduce platform features in too technically correct and unintuitive a way.
There was a problem hiding this comment.
I see this as reference material, not top-to-bottom read material, and I expect hierarchical nav to represent that. Workflow utilities go with workflows, activity utilities go with activities, etc. I don't expect to have workflow utility reference material fragmented as you don't have anywhere to go to see all workflow utility pieces. I don't expect to have a futures section for child workflows, nexus operations, etc even though they apply to those too.
But these are just my opinions on how I see these docs vs something on learn.temporal.io.
There was a problem hiding this comment.
I think that's a good argument, but it's one we've been having here for years, and in practice, I think we wind up continually returning to the reality that people are approaching it 50-50 as reference material and top-to-bottom material. Notably, one place this PR does already deviate from the others in a way I didn't comment on today is by adding a Quickstart page to the top of the Ruby SDK section -- I thought this was a neat experiment that we'd build on -- in light of that finding. This is, imo, one of the only places where we're seriously violating an assumption that people might be top-to-bottom, hence why I'm drawing attention to this one.
| However, there is a limit to the total size of the data that ends up encoded into a gRPC message Payload. | ||
|
|
||
| A single argument is limited to a maximum size of 2 MB. | ||
| And the total size of a gRPC message, which includes all the arguments, is limited to a maximum of 4 MB. |
There was a problem hiding this comment.
I'd add a sentence here (to all of the SDKs if the explanation is consistently this dry) that says, "it is almost never necessary to send large binary data as input and output to your Workflows. If this is ever required for your business logic, you can implement an S3 storage hook following this example: https://github.com/DataDog/temporal-large-payload-codec" or something along those lines. I think this limit is a bit confusing out of context otherwise.
There was a problem hiding this comment.
I would like us to not recommend large payload codec, it is not a good pattern and has many problems. We admittedly lack a good blog post or docs on how best to handle this (can discuss internally on prioritizing that), but we should not encourage large payload codec.
There was a problem hiding this comment.
Fine by me, in that case I revise my feedback to clarifying that you can usually just send strings or identifiers instead of needing to send large blobs as I/O
| And the total size of a gRPC message, which includes all the arguments, is limited to a maximum of 4 MB. | ||
|
|
||
| Also, keep in mind that all Payload data is recorded in the [Workflow Execution Event History](/workflow-execution/event#event-history) and large Event Histories can affect Worker performance. | ||
| This is because the entire Event History could be transferred to a Worker Process with a [Workflow Task](/tasks#workflow-task). |
There was a problem hiding this comment.
I would consider just removing this paragraph, I don't think it belongs this early in the SDK docs and it's a bit scary. Should be an encyclopedia concept only.
There was a problem hiding this comment.
The struggle I had is that we are trying to combine "fix general SDK docs" and "make Ruby SDK docs" into one. Can we do the general SDK doc improvement that applies to multiple SDKs a separate PR? Ideally we should get Ruby like others, and fix all others if there is a problem with all others. This makes it easier to port.
There was a problem hiding this comment.
Generally yes, and that's the model I've followed with #3598, but you might be interested to know that I've gotten just as much feedback from people who don't like that model either, and there are a lot of clarity improvements we can make here that would not have gotten looked at if not for this porting opportunity. I am happy to add the other SDK page changes in this PR but I think people would then start talking about scope creep and blocking Ruby...
There was a problem hiding this comment.
In general at least with code we don't like to mix refactoring and greenfield work because the refactoring gets lost in the details. If we have things we need to improve across SDK docs, it should be improved across SDK docs IMO separately from a single language migration.
Sounds like several of these things need to be improved generally and a PR should be opened for those unrelated to Ruby. The SDK docs gain more and more inconsistency if we apply certain improvements only to specific languages and leave them in others.
|
|
||
| Some SDKs require that you pass context objects, others do not. | ||
| When it comes to your application data—that is, data that is serialized and encoded into a Payload—we recommend that you use a single hash or object as an argument that wraps the application data passed to Activities. | ||
| This is so that you can change what data is passed to the Activity without breaking a method signature. |
There was a problem hiding this comment.
Should have an example here using a Ruby object rather than just talking about it
There was a problem hiding this comment.
We chose not to do this in other SDK docs. If we want to consistently provide this across SDKs, I think we should do so across SDKs and not make Ruby unique here.
|
|
||
| Activity parameters are the method parameters of the `execute` method. | ||
| These can be any data type Temporal can convert. | ||
| Technically this can be multiple parameters, but Temporal strongly encourages a single parameter containing all input fields. |
There was a problem hiding this comment.
This is redundant with the preceding paragraph
There was a problem hiding this comment.
This is also the case in other SDK docs. Are you sure you only want to improve here and not there?
| These can be any data type Temporal can convert. | ||
| Technically this can be multiple parameters, but Temporal strongly encourages a single parameter containing all input fields. | ||
|
|
||
| ### Activity Concurrency and Executors {#activity-concurrency-and-executors} |
There was a problem hiding this comment.
This should probably also go under "Start Activity Execution" as an H4, and maybe even in an admonition to clarify that these are advanced threading concepts that people will not need as they're starting out.
There was a problem hiding this comment.
This section is very much for implementation/definition of activities. The starting of activities is unrelated to this implementation aspect.
| The only required value that needs to be set is either a [Schedule-To-Close Timeout](/encyclopedia/detecting-activity-failures#schedule-to-close-timeout) or a [Start-To-Close Timeout](/encyclopedia/detecting-activity-failures#start-to-close-timeout). | ||
| These values are set as keyword parameters. | ||
|
|
||
| ### Get Activity Execution results {#get-activity-results} |
There was a problem hiding this comment.
I would remove this H4 and just keep the sentence beneath it.
There was a problem hiding this comment.
Does this only apply to Ruby docs?
There was a problem hiding this comment.
See #3560 (comment), and tell me which method of improvement you would object to least :)
| The call to spawn an Activity Execution generates the [ScheduleActivityTask](/references/commands#scheduleactivitytask) Command. | ||
| This results in the set of three [Activity Task](/tasks#activity-task) related Events ([ActivityTaskScheduled](/references/events#activitytaskscheduled), [ActivityTaskStarted](/references/events#activitytaskstarted), and ActivityTask[Closed])in your Workflow Execution Event History. | ||
|
|
||
| Activity implementation code should be _idempotent_. |
There was a problem hiding this comment.
This should be under Develop an Activity, not Start Activity Execution, and it should have a link to another part of our docs explaining this...
|
|
||
| - Each [Worker Entity](/workers#worker-entity) in the Worker Process must register the exact Workflow Types and Activity Types it may execute. | ||
| - Each Worker Entity must also associate itself with exactly one [Task Queue](/task-queue). | ||
| - Each Worker Entity polling the same Task Queue must be registered with the same Workflow Types and Activity Types. |
There was a problem hiding this comment.
These bullet points should be next to the little code sample down below and softened somewhat, otherwise it feels like we're giving people a lot of scary / arbitrary rules before they've seen how task queue assignment actually works.
There was a problem hiding this comment.
Does this only apply to Ruby docs? Can we do a separate SDK docs review not specific to Ruby?
|
|
||
| A [Worker Entity](/workers#worker-entity) is the component within a Worker Process that listens to a specific Task Queue. | ||
|
|
||
| Although multiple Worker Entities can be in a single Worker Process, a single Worker Entity Worker Process may be perfectly sufficient. |
There was a problem hiding this comment.
I would cut this from this guide, doesn't make any sense to anyone who hasn't already gone deeper into scaling and deploys.
|
|
||
| A Worker Entity contains a Workflow Worker and/or an Activity Worker, which makes progress on Workflow Executions and Activity Executions, respectively. | ||
|
|
||
| To develop a Worker, create a new `Temporalio::Worker` providing the Client and worker options which include Task Queue, Workflows, and Activities and more. |
There was a problem hiding this comment.
I think the word here shouldn't necessarily be "develop" because you don't need to do a lot of development to ship a Worker. Maybe something like:
| To develop a Worker, create a new `Temporalio::Worker` providing the Client and worker options which include Task Queue, Workflows, and Activities and more. | |
| Workers are implemented in each Temporal SDK, and can be deployed with just a bit of boilerplate. | |
| To create a Worker, use `Temporalio::Worker.new()`, providing the Worker options which include Task Queue, Workflows, and Activities and more. |
|
Thank you, axfelix! 🙌 I'll go through these today/tomorrow. |
Co-authored-by: Alex Garnett <axfelix@gmail.com>
Co-authored-by: Alex Garnett <axfelix@gmail.com>
Co-authored-by: Alex Garnett <axfelix@gmail.com>
Co-authored-by: Alex Garnett <axfelix@gmail.com>
What does this PR do?
Notes to reviewers