Memos #121

jeffschoner-stripe · 2021-11-18T01:19:34Z

Exposes memo to a running workflow through workflow.metadata.memo
Adds options to specify a memo when starting or scheduling a workflow
Some additional refactoring of mapping between proto and JSON maps for memos which is also applied to existing headers that have the same structure

Testing

There are new and updated unit specs, including changes to fabricators
The integration test for MetadataWorkflow now also checks for memo round-tripping

antstorm · 2021-11-21T14:21:44Z

This should now be rebaseable

jeffschoner-stripe · 2021-11-22T16:29:18Z

@antstorm This has been rebased, cleaned up, and is ready for final review

antstorm · 2021-11-25T16:28:35Z

lib/temporal/connection/serializer/continue_as_new.rb

+        def serialize_memo(memo)
+          return unless memo
+
+          Temporal::Api::Common::V1::Memo.new(fields: object.memo)


I'm assuming we need to call to_payload_map here to properly serialise it. Also we should use the local memo variable, not object.memo

I'll see if I can cover this in some tests, and clean up the mixed instance/local variable pattern I blindly copied from headers.

This should now be fixed for all memos. I'll do the same for headers in a separate PR.

antstorm · 2021-11-25T16:28:47Z

lib/temporal/connection/serializer/start_child_workflow.rb

+        def serialize_memo(memo)
+          return unless memo
+
+          Temporal::Api::Common::V1::Memo.new(fields: object.memo)


same here ^

antstorm · 2021-11-25T16:30:04Z

lib/temporal/connection/serializer/start_child_workflow.rb

        def serialize_headers(headers)
          return unless headers

          Temporal::Api::Common::V1::Header.new(fields: object.headers)


nit: This is probably out of scope for this PR, but this also needs to have to_payload_map call

antstorm · 2021-11-25T16:31:56Z

lib/temporal/workflow/execution_info.rb

  class Workflow
-    class ExecutionInfo < Struct.new(:workflow, :workflow_id, :run_id, :start_time, :close_time, :status, :history_length, keyword_init: true)
+    class ExecutionInfo < Struct.new(:workflow, :workflow_id, :run_id, :start_time, :close_time, :status, :history_length, :memo, keyword_init: true)
+      extend Concerns::Payloads


This needs to be required within this file

jeffschoner-stripe · 2021-11-25T23:49:11Z

@antstorm I believe I've addressed the latest round of feedback regarding memos.

I have the changes to improve headers on another branch ready to be rebased onto this PR once it's merged too. The change I'll be making to headers will be a breaking one in that callers will need to pass a plain hash of header key/value pairs rather than constructing the generated proto that's required today. This should be much more ergonomic but will benefit from a dedicated PR that calls out the impact of that change.

jeffschoner-stripe · 2021-11-25T23:54:14Z

lib/temporal/workflow/context.rb

          retry_policy: execution_options.retry_policy,
-          headers: execution_options.headers
+          headers: execution_options.headers,
+          memo: execution_options.memo,


This is also new in this latest iteration. The ensures the memo is copied in a continue-as-new.

jeffschoner-stripe · 2021-11-25T23:55:14Z

examples/workflows/loop_workflow.rb

 class LoopWorkflow < Temporal::Workflow
-  def execute(count)
+  def execute(count, assert_memo)
+    assert_memo.each do |memo_key, memo_value|


Not sure how you feel about this approach. Essentially, I need the workflow to check its own memo value on each iteration continues to match what was passed to the first iteration.

I ended up scrapping this, having it return count and memo, and instead created a dedicated integration test for continue-as-new behavior. This also uncovered that memo was not being automatically copied to the next iteration as well, which is the behavior in Java and Go SDKs.

jeffschoner-stripe · 2021-11-26T19:21:50Z

Made a few more changes to how the integration tests are structured, and copying memos to the next iteration of a continue-as-new workflow by default.

antstorm

Looks good, thank you!

@option

* Pass config to the error handler instead of using the global config * Fix example tests * Make the config property in the workflow context publicly readable * [Fix] Retryer GRPC error lookup (coinbase#109) * Fix issue with GRPC error lookup in Retryer * Rename spec file for retryer to contain _spec * [Feature] Add id and domain to workflow context's metadata (coinbase#110) * Start syncing id and domain on workflow context metadata * Fixed tests Co-authored-by: DeRauk Gibble <derauk.gibble@coinbase.com> * Explicit docker-compose project name (coinbase#114) * Add YARD documentation for Temporal::Client (coinbase#113) * Add YARD documentation for Temporal::Client * Add yard gem * Fix @option tag * Typo fix * Add signal arguments to start_workflow (support for signal_with_start) (coinbase#112) * Add signal arguments to start_workflow (to support signal_with_start) * Move signal arguments to the options hash * PR feedback * Fix merge error * Extend #wait_for to take multiple futures and a condition block (coinbase#111) * Differentiate TARGET_WILDCARD and WILDCARD, allow comparison with EventTarget objects (coinbase#118) * Turn off schedule_to_start activity timeout by default (coinbase#119) * Separate options from keyword args in #start_workflow (coinbase#117) * Separate options from keyword args in #start_workflow * fixup! Separate options from keyword args in #start_workflow * Surface additional workflow metadata on workflow context (coinbase#120) * Refactor metadata generation * Make task queue available on workflow metadata, add example test * Expose workflow start time metadata * Add memos (coinbase#121) * Add describe_namespace (coinbase#122) * Add describe_namespace * Feedback * Improve header serialization and propagation (coinbase#124) * [Fix] Non-started activity cancellation (coinbase#125) * Fix event target map entry for ACTIVITY_CANCELED event * Fix cancellation of a non-started activity * fixup! Fix event target map entry for ACTIVITY_CANCELED event Co-authored-by: DeRauk Gibble <derauk.gibble@coinbase.com> Co-authored-by: DeRauk Gibble <derauk@gmail.com> Co-authored-by: Anthony Dmitriyev <antstorm@gmail.com> Co-authored-by: nagl-stripe <86737162+nagl-stripe@users.noreply.github.com> Co-authored-by: jeffschoner-stripe <63118764+jeffschoner-stripe@users.noreply.github.com> Co-authored-by: Drew Hoskins <37816070+drewhoskins-stripe@users.noreply.github.com>

…oner-search-attributes APIs for adding, listing, and removing custom search attributes

jeffschoner-stripe mentioned this pull request Nov 18, 2021

Surface additional workflow metadata on workflow context #120

Merged

jeffschoner-stripe force-pushed the memos branch from 36b45a6 to 2a88df9 Compare November 22, 2021 16:24

jeffschoner-stripe changed the title ~~[WIP] Memos~~ Memos Nov 22, 2021

antstorm reviewed Nov 25, 2021

View reviewed changes

jeffschoner-stripe force-pushed the memos branch from 2a88df9 to 2059bda Compare November 25, 2021 23:43

jeffschoner-stripe commented Nov 25, 2021

View reviewed changes

jeffschoner-stripe force-pushed the memos branch from 2059bda to c858941 Compare November 26, 2021 19:12

Add memos

504598b

jeffschoner-stripe force-pushed the memos branch from c858941 to 504598b Compare November 26, 2021 19:17

antstorm approved these changes Nov 28, 2021

View reviewed changes

antstorm merged commit 7d63692 into coinbase:master Nov 28, 2021

jeffschoner-stripe deleted the memos branch November 29, 2021 07:05

aryak-stripe mentioned this pull request Mar 7, 2022

[wip] expose memo for child workflow execution #160

Merged

jazevedo-stripe mentioned this pull request Jul 18, 2022

Allow initial search attributes to be specified when starting workflows #188

Merged

calum-stripe pushed a commit to calum-stripe/temporal-ruby that referenced this pull request Mar 28, 2023

Merge pull request coinbase#121 from stripe-private-oss-forks/jeffsch…

94cd205

…oner-search-attributes APIs for adding, listing, and removing custom search attributes

emmmbrown mentioned this pull request Feb 16, 2024

Merge upstream (#5) clearbit/temporal-ruby#7

Closed

Memos #121

Memos #121

Uh oh!

Conversation

jeffschoner-stripe commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

antstorm commented Nov 21, 2021

Uh oh!

jeffschoner-stripe commented Nov 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffschoner-stripe commented Nov 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffschoner-stripe commented Nov 26, 2021

Uh oh!

antstorm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

jeffschoner-stripe commented Nov 18, 2021 •

edited

Loading