Skip to content

Conversation

@jeffschoner-stripe
Copy link
Contributor

@jeffschoner-stripe jeffschoner-stripe commented Nov 12, 2021

Motivation

During registration, the Dispatcher places all dispatch targets into a hash keyed on target. Before #111, event targets were always of type EventTarget. With that change, event targets could also be of type String (i.e., "*"). In the implementation of Hash, objects of these two types can be compared using the eql? method when they end up in the same bucket behind the scenes. EventTarget.eql? can only compare objects that have an id and type field like itself. Because a String does not have these fields, it sometimes fails with a specific error while trying to register a dispatch handler:

Failure/Error: id == other.id && type == other.type

     NoMethodError:
       undefined method `id' for "*":String
     # ./lib/temporal/workflow/history/event_target.rb:65:in `=='
     # ./lib/temporal/workflow/history/event_target.rb:70:in `eql?'
     # ./spec/unit/lib/temporal/workflow/dispatcher_spec.rb:89:in `block (4 levels) in <top (required)>'

Summary

  • Modifies EventTarget.eql? to return false when called with an object that is not of its own type
  • Separates TARGET_WILDCARD from WILDCARD to more clearly indicate their separate functions
  • The dispatcher unit tests have been updated to use EventTarget objects rather than Strings to reflect the actual type used in the implementation outside of the tests
  • A new unit test has been added to ensure EventType can be compared against TARGET_WILDCARD

Testing

Updated and new tests can all be run with,

rspec spec/unit/lib/temporal/workflow/dispatcher_spec.rb

Copy link
Contributor

@antstorm antstorm left a comment

Choose a reason for hiding this comment

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

Great catch! Thank you for noticing and quickly fixing it

@antstorm antstorm merged commit 388d9ed into coinbase:master Nov 15, 2021
@jeffschoner-stripe jeffschoner-stripe deleted the dispatch-id-match branch November 15, 2021 17:34
gvieira pushed a commit to clearbit/temporal-ruby that referenced this pull request Dec 21, 2021
* 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>
calum-stripe added a commit to calum-stripe/temporal-ruby that referenced this pull request Mar 28, 2023
…dding-page-size

added max page size param
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants