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

deliver broadcast messages asynchronously #41

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prog-supdex
Copy link
Contributor

Deliver messages, which are called by method trigger, asynchronously

It`s a not final PR

@@ -30,11 +30,13 @@ Gem::Specification.new do |spec|
spec.add_dependency "anyway_config", ">= 1.3", "< 3"
spec.add_dependency "graphql", ">= 1.11", "< 3"
spec.add_dependency "redis", ">= 4.2.0"
spec.add_dependency "activejob", ">= 5.2.0"
Copy link
Member

Choose a reason for hiding this comment

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

It shouldn't be a dependency; the gem is Rails-free, we only provide ActiveJob integration if ActiveJob is available in the runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it to spec.add_development_dependency "activejob", ">= 6.0.0"

because otherwise, I got an error during running specs

@@ -6,6 +6,7 @@
require_relative "graphql/anycable/cleaner"
require_relative "graphql/anycable/config"
require_relative "graphql/anycable/railtie" if defined?(Rails)
require_relative "graphql/adapters/base_job"
Copy link
Member

Choose a reason for hiding this comment

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

Move into graphql/anycable/railtie and load only if ActiveJob is present (using ActiveSupport.on_load(:active_job) { ... } hook).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I done it, but I had an error in this case

initializer 'graphql_anycable.load_trigger_job' do
  ActiveSupport.on_load(:active_job) do
    require "graphql/adapters/trigger_job"
  end
end

in that case, my application (rails 6, with configurated active_job) does not load it, because my application does not call any ActiveJob jobs in the code

I checked and saw, that ActiveSupport.on_load(:active_job) loads only when my rails application calls any jobs

but, maybe I misunderstood something.

Currently, I just commented ActiveSupport.on_load(:active_job) and I think, if we are in Railtie, we can require TriggerJob in initialize block

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

I'm looking at how ActionMailer or ActiveStorage does this, and it seems that we can just add require "active_job/railtie" and load our job class after that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for that!

The trouble was with my test application

class BaseJob < ActiveJob::Base
DEFAULT_QUEUE_NAME = :default

queue_as { GraphQL::AnyCable.config.async_broadcasting["queue"] || DEFAULT_QUEUE_NAME }
Copy link
Member

Choose a reason for hiding this comment

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

The default name must be define in the configuration, no need to use a fallback here:

Suggested change
queue_as { GraphQL::AnyCable.config.async_broadcasting["queue"] || DEFAULT_QUEUE_NAME }
queue_as do
AnyCable.config.active_job_queue_name
end

Copy link
Member

Choose a reason for hiding this comment

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

Or, we go with adapter instances, we can specify the queue right when calling #perform_later:

TriggerJob.set(queue: queue).perform_later(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
Done

private

def schema_parse(serialized_payload)
payload = JSON.parse(serialized_payload)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe, we can use symbolize_names here and avoid #transform_keys?

Suggested change
payload = JSON.parse(serialized_payload)
payload = JSON.parse(serialized_payload, symbolize_names: true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, yeah. We can just put symbolized hash to ActiveJob arguments, and it will work without JSON.parse

Comment on lines 16 to 17
attr_config use_async_broadcasting: true
attr_config async_broadcasting: { queue: "broadcasting", class: "GraphQL::Adapters::BaseJob" }
Copy link
Member

Choose a reason for hiding this comment

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

This is not how adapter pattern is implemented. The API must be as follows:

GraphQL::AnyCable.delivery_method = :inline

GraphQL::AnyCable.delivery_method = :active_job

# or with options

GraphQL::AnyCable.delivery_method = :active_job, queue: "custom_queue", job_class: "MyJob"

And then, in the #trigger method we simply delegate the execution to the configured adapter instance. Smth like:

def trigger(...)
  AnyCable.delivery_adapter.trigger(...)
end

Copy link
Contributor Author

@prog-supdex prog-supdex Sep 26, 2023

Choose a reason for hiding this comment

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

I did it, but without "="

It works like that

GraphQL::AnyCable.delivery_method("active_job", queue: "custom_queue", job_class: "MyJob")

But, if we need like setter, we will able to call it only like that (adds { and } for options)

GraphQL::AnyCable.delivery_method = :active_job, {queue: "custom_queue", job_class: "MyJob"}

Because the setter, as I know, cannot accept keyword arguments

GraphQL::AnyCable.delivery_method = "inline", queue: "queue", job_class: "Job"
SyntaxError ((irb):7: syntax error, unexpected tLABEL)
...ble.delivery_method = 1, queue: 123, job_class: "Abc"
...                         ^~~~~~

Copy link
Member

Choose a reason for hiding this comment

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

if we need like setter, we will able to call it only like that

Yes, and that's how it should be done. That's a common pattern used by Rails and many libraries, We shouldn't reinvent it.

So, please, implement it using the original proposed interface.

P.S. See, for example: https://github.com/palkan/active_delivery/blob/master/lib/abstract_notifier/async_adapters.rb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mm, yea, I earlier said, why I did not do that

I fixed it, but it will work only with

GraphQL::AnyCable.delivery_method = :active_job, {queue: "custom_queue", job_class: "MyJob"}

so, we need to wrap by { and } last arguments

end

executor_class_job.perform_later(
serialized_arguments,
Copy link
Member

Choose a reason for hiding this comment

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

We should not serialize arguments; it's the responsibility of Active Job; we may assume that arguments are GlobalID identifiable, so we can pass them as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, currently I do not serialize them, but I still collected them (arguments)


return Adapters::BaseJob unless custom_class

Object.const_get(config.async_broadcasting["class"])
Copy link
Member

Choose a reason for hiding this comment

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

The job class is inferred in Rails context where we can use #safe_constantize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!
But for job_class, I put #constantize instead of #safe_constantize, because I think that #constantize will be useful for cases when the user puts anything wrong to param job_class.
And constantize will raise an error and we will see it immediately

But, if I am wrong, I will fix it

Object.const_get(config.async_broadcasting["class"])
end

def perform(event, object)
Copy link
Member

Choose a reason for hiding this comment

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

Where do we use this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was pre-alfa version of the code, sorry :)

require "active_job"

RSpec.describe GraphQL::Adapters::BaseJob, type: :job do
ActiveJob::Base.queue_adapter = :inline
Copy link
Member

Choose a reason for hiding this comment

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

Never define configuration outside of before and after hooks, and always restore the value after each test. Otherwise it's easier to introduce unpredictable global state and flakiness.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the advice!

@prog-supdex prog-supdex marked this pull request as ready for review September 27, 2023 07:59
@prog-supdex prog-supdex force-pushed the async_triggers branch 3 times, most recently from 79c4a14 to 1dee021 Compare September 27, 2023 13:09
@executor_method = executor_object.class::EXECUTOR_METHOD_NAME
end

def trigger(...)
Copy link
Contributor Author

@prog-supdex prog-supdex Sep 27, 2023

Choose a reason for hiding this comment

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

I use here "...", but It requires "ruby-2.7".

I checked ci-cd, and I saw minimal ruby versions - 2.7, but if we look at the documentation (in anycable-rails, because it is a dependency), I see the requirement for ruby-version 2.6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@palkan please tell me, do we need to consider ruby 2.6 and avoid using "..." ?
If yes, I will fix it

@@ -6,6 +6,7 @@
require_relative "graphql/anycable/cleaner"
require_relative "graphql/anycable/config"
require_relative "graphql/anycable/railtie" if defined?(Rails)
require_relative "graphql/adapters/base_job"
Copy link
Member

Choose a reason for hiding this comment

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

Got it.

I'm looking at how ActionMailer or ActiveStorage does this, and it seems that we can just add require "active_job/railtie" and load our job class after that.

@@ -3,31 +3,19 @@
module GraphQL
module Adapters
class DeliveryAdapter
Copy link
Member

Choose a reason for hiding this comment

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

The goal of adapter patter is to provide extensibility. Moving all different implementations into a single file and calling it an "adapter" doesn't make it an adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, I didn`t understand what was needed, but I now got it.

Now we can easily add any other adapter with his logic and add it in config.delivery_method

Thanks for the lesson

Comment on lines 16 to 17
attr_config use_async_broadcasting: true
attr_config async_broadcasting: { queue: "broadcasting", class: "GraphQL::Adapters::BaseJob" }
Copy link
Member

Choose a reason for hiding this comment

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

if we need like setter, we will able to call it only like that

Yes, and that's how it should be done. That's a common pattern used by Rails and many libraries, We shouldn't reinvent it.

So, please, implement it using the original proposed interface.

P.S. See, for example: https://github.com/palkan/active_delivery/blob/master/lib/abstract_notifier/async_adapters.rb

@prog-supdex prog-supdex force-pushed the async_triggers branch 4 times, most recently from b9a692a to ebc7e13 Compare October 4, 2023 17:32
@prog-supdex
Copy link
Contributor Author

@palkan hi!

I sum up the changes

Currently, we have two adapters, "Inline" and "ActiveJob" in two files

The specific adapter is chosen by DeliveryAdapter.lookup and settings in the configuration (delivery_method)

Also, we have a custom serializer AnyCableSubscriptionSerializer which helps do not think about an instance of class AnyCableSubscriptions in active jobs, for example here

But for example here I used the arguments forwarding shorthand, which exists in ruby 2.7 (I see that ci-cd checks on ruby 2.7 and upper), but this gem has a dependency anycable-rails, which requires minimum 2.6 ruby

Maybe here I need not to use the arguments forwarding shorthand

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants