Skip to content

Conversation

@EiNSTeiN-
Copy link

This PR add callbacks around each iteration. The intention is for concerns to use this to inject behaviours around iterations.

The first use-case I see for this is to clear the ActiveRecord query cache between iterations of the BackgroundQueue::LockQueue concern, because not doing so may change the result of each iteration of a job compared to running multiple jobs.

@EiNSTeiN-
Copy link
Author

test failures are unrelated

@kirs
Copy link
Contributor

kirs commented Nov 20, 2018

We're thinking about getting rid of callbacks in the next versions (#19) as their behaviour can be confusing (do you expect on_shutdown run before or after on_complete?).

For the use case to drop query cache between iterations, could you use something like this?

class ResetQueryCacheEnumerator
  def initialize(enum)
    @enum = enum
  end

  def to_enum
    Enumerator.new do |yielder|
       @enum.each do |*payload|
         yielder.yield(*payload)
         ActiveRecord::Base.reset_query_cache # whatever cleanup here
       end
    end
  end
end

def build_enumerable
  enum = enumerator_builder.lock_queue(...)
  ResetQueryCacheEnumerator.new(enum).to_enum
end

This is a lot better composable than callbacks and even gives you a way to test ResetQueryCacheEnumerator separately from the job.

We use this enumerator wrapping for many use cases like throttling and progress updates.

@kirs
Copy link
Contributor

kirs commented Nov 20, 2018

cc @GustavoCaso

@EiNSTeiN-
Copy link
Author

their behaviour can be confusing (do you expect on_shutdown run before or after on_complete?).

That's what documentation is for! Callbacks are used in a lot of jobs so I am doubtful that it is realistic to remove them.

could you use something like this?

That would work nicely if all jobs used our iteration pattern. In reality not all jobs use it, some jobs define a perform method with a simple lock_queue.each loop. A concern might want to provide automatic behaviour (like logging) by just being included, without each job needing to use the right combination of enumerators too. I'm afraid that requiring each job to wrap enumerators with other enumerators will be fragile compared to automatically providing the right behaviour.

@GustavoCaso
Copy link
Contributor

@EiNSTeiN- Yeah we will try finding a way to remove them, but so far no idea how. But I'm with @kirs I think is best to implement this as a separate Enumerator.

Thanks

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.

3 participants