Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reevaluate rollout of enforced cursor serializability #80

Open
sambostock opened this issue Apr 20, 2021 · 2 comments · May be fixed by #81
Open

Reevaluate rollout of enforced cursor serializability #80

sambostock opened this issue Apr 20, 2021 · 2 comments · May be fixed by #81
Assignees
Labels
enhancement New feature or request

Comments

@sambostock
Copy link
Contributor

Context

The enforcement that cursors be losslessly serializable to JSON introduced in #73 was released in 1.1.11.

Unfortunately, this broke some production jobs, due to excessive mocking in tests not catching unserializable cursors.

We have since reverted the change in #77 and published version 1.1.12.

This was discussed in Slack:

  • Apps may be relying on the existing behaviour of being able to provide arbitrary cursor and dealing with deserializing the corrupted cursor.
  • Those apps would now encounter an error
  • This is therefore a breaking change, and would likely require a major version upgrade
  • Tests do not always discover breakages, so it may be desirable to be able to customize the behaviour in the event of an unserializable cursor.

Way Forward

We could reintroduce the cursor validation, but this time with behaviour customizable by the host app.

  • By default, we could simply log a warning via Rails.logger.warn.
  • Host apps wishing to be more strict could select alternative behaviour:
    JobIteration.configure do |config|
      config.on_cursor_unsafe_to_serialize = :raise
    end
  • Host apps requiring even more customized behaviour could register a callable
    JobIteration.configure do |config|
      config.on_cursor_unsafe_to_serialize = lambda do |job, cursor|
        # e.g. compare job against some list and conditionally raise
        # e.g. emit a metric to some metric service
      end
    end

This could be shipped as part of a minor version bump, as it would be a non-breaking new feature intended to provide more robust guarantees.

When we decide it is appropriate to change the default, we can simply change it and release a major version bump. Consumers could continue to override the config until such a time as we decide to get rid of it.

@sambostock sambostock self-assigned this Apr 20, 2021
@rafaelfranca
Copy link
Member

I think I'd just go with a deprecate current behavior and in the next version remove it. If apps want to deal with complex objects they can do directly in the caller. I don't think we need a configuration for this.

@sambostock sambostock added the enhancement New feature or request label Apr 20, 2021
@sambostock
Copy link
Contributor Author

Sounds good. Is there good way to leverage ActiveSupport::Deprecation for this, @rafaelfranca?

I was looking at ActiveSupport::Deprecation::Reporting#deprecation_warning, but it expects a deprecated_method_name. We aren't getting rid of a method, we're going to raise an error on unsupported behaviour.

D = ActiveSupport::Deprecation.new('2.0', 'JobIteration')
D.deprecation_warning(:build_enumerator, 'unsafe cursor message...')
# DEPRECATION WARNING: build_enumerator is deprecated and will be removed from JobIteration 2.0 (unsafe cursor message...) (called from <top (required)> at /opt/rubies/2.6.5/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11)

@sambostock sambostock linked a pull request Apr 21, 2021 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants