Closed davehughes closed 1 month ago
@davehughes Thank you for the PR! Could you please rebase onto master to get the test fix for CI?
Bump?
@DeRauk thanks!
@davehughes @DeRauk I'm not sure this is working as expected. I believe the value set in the configuration instance will always be used in grpc.rb
. The :use_error_serialization_v2
option cannot be unset such that it will fall back to the global. Setting it to false
or nil
does not seem to change this behavior.
E.g.,
irb(main):001:0> options = {}
=> {}
irb(main):002:0> options.fetch(:use_error_serialization_v2, true)
=> true
irb(main):003:0> options = {}.merge(use_error_serialization_v2: nil)
=> {:use_error_serialization_v2=>nil}
irb(main):004:0> options.fetch(:use_error_serialization_v2, true)
=> nil
irb(main):005:0> options = {}.merge(use_error_serialization_v2: false)
=> {:use_error_serialization_v2=>false}
irb(main):006:0> options.fetch(:use_error_serialization_v2, true)
=> false
Reading this from the options that are passed in from the configuration is still the right way to go, rather than relying on global configuration being read via Temporal.configuration
.
This change, however, does make a breaking change for anyone who has opted into error serialization v2 via global config.
What do you think about changing this from
options.fetch(:use_error_serialization_v2, Temporal.configuration.error_serialization_v2)
to
options[:use_error_serialization] || Temporal.configuration.error_serialization_v2
?
Hmm...I see what you mean and how this is an accidentally breaking change for a set of scenarios.
My mental model of configuration usage here is that folks should either choose to configure via the global singleton or through local configuration objects depending on their needs. I'm not sure how strongly the project wants to hold to the semantics that the global config is a fallback vs. a distinct thing, though clearly there's some of the former going on.
I don't think the proposed change works as written, since if options[:use_error_serialization] == false
, the code will always fall back to the global setting. But allowing nil
values and falling back in the presence of a nil
should work:
use_v2 = options[:use_error_serialization_v2]
use_v2 = Temporal.configuration.error_serialization_v2 if use_v2.nil?
In conjunction with using a nil
default in the configuration constructor here should allow the global fallback to apply as you expect.
All that said, I'm really just a nomad passing through this project. I've left the job where I noticed the issue that led to this PR, and fixing this is not really a priority for me. (Although I do apologize for the inadvertent breakage, and would be happy to shepherd through a PR with the approach above if asked.)
Attempt to address https://github.com/coinbase/temporal-ruby/issues/295.
Alter
Temporal::Connection::GRPC
to respect a:use_error_serialization_v2
option, falling back to the global singleton value if not found, and updateTemporal::Configuration#for_connection
to pass its setting through when creating connections.It looks like options can already be plumbed through by editing a Temporal::Configuration instance's
connection_options
, but this case seems slightly different becauseuse_error_serialization_v2
is an explicit setting on the configuration.