Sqewer::SimpleJob doesn't help adding new attributes to a Job class

[fabioperrella]

Sometimes, we want to add a new attribute to a Job class, which includes Sqewer::SimpleJob, but it's not possible because Sqewer::SimpleJob.new raises an error when it detects that there are missing attributes.

Example:

Assuming that exists the following job:

class MyJob
  include Sqewer::SimpleJob
  attr_accessor :width

  def run
   puts 'run'
  end
end

And we want to add the attribute height:

class MyJob
  include Sqewer::SimpleJob
  attr_accessor :width, :height

  def run
   puts 'run'
  end
end

By doing this, when the new version is deployed, if there are old jobs enqueued, it will raise MissingAttribute because of this piece of code:

# lib/sqewer/simple_job.rb
accessors = methods.grep(EQ_END).map{|method_name| method_name.to_s.gsub(EQ_END, '\1').to_sym }
settable_attributes = Set.new(accessors)
missing_attributes = settable_attributes - touched_attributes

missing_attributes.each do | attr |
  raise MissingAttribute, "Missing job attribute #{attr.inspect}"
end

So, the only option to add a new attribute is to add a new job, duplicating the previous one, for example:

class MyJobV2
  include Sqewer::SimpleJob
  attr_accessor :width, :height

  def run
   puts 'run'
  end
end

This has the advantage of being safer when changing the job's payload, but it complicates the analysis if the PR because a whole new file has been added.

My suggestion is adding a method to allow changing this behaviour, such as:

class MyJob
  include Sqewer::SimpleJob
  allow_missing_attributes

  attr_accessor :width, :height

By doing this we can use the new attribute assuming that it may not exist, for example:

class MyJob
  include Sqewer::SimpleJob
  allow_missing_attributes # <-- new method

  attr_accessor :width, :height

 def run
   if height.present?
     # do something
   end
 end
end

wdyt?

cc @julik @linkyndy @nitika080289 @martijnvermaat @lorenzograndi4

[lorenzograndi4]

I'm in favour, spawning new jobs every time is a pain!

[linkyndy]

While I do see changes like this as pretty cumbersome, I don't think they outweigh safety and the overall reliability of the library. Runtime issues caused by wrong arguments passed are more frequently to occur than the need to alter a job's interface. Therefore, I do see the reasoning for why it has been designed like this.

I believe we could improve this, though I am not sure your proposed solution should be the way. For simplicity, I would either keep the contract strict, or would remove the need for it and rely on developer's common sense (in the Sidekiq way). Mixing the two would only cause confusion when using the library, imho.

[fabioperrella]

My idea is not to change the standard as it is today, because even if it is not a compatibility break, it would change a behavior that some developers are used to.

Therefore, I'd prefer to make the opt-in explicit.

[martijnvermaat]

I understand @linkyndy 's concern, but if we clearly document the intended use case I think this could be a good approach.

For example, the README could include a section like this:

Adding job attributes

By default, Sqewer raises MissingAttribute if an expected attribute on the job is missing. This is safe [bladiebla...] . However, it makes adding new attributes to existing jobs painful by having to effectively create a duplicate of the job with the new attribute.

For this, Sqewer provides allow_missing_attributes. The suggestion is to use is it only when adding new attributes, and removing it again when the change is fully deployed to restore safety benefits from the attribute check.

[linkyndy]

To me, this sounds more confusing (I am a fan of simplicity and of having a single, clear way of doing things). And if one anyway would have to add something only to remove it later, it doesn't improve the process by much. But if you feel this would really have a positive impact, don't get blocked by me 🙂

[julik]

Imagine one worker (let's call it "Worker O") has Job{width, height}. Another worker (which we'll call "Worker N") has Job{width, height, bit_depth}. Imagine "Worker O" receives a Job with a bit depth set, but doesn't know that this attribute exists. It then tries to execute the job, but for unrelated reasons it has to spool the job for retry (say it gets terminated, or Fluxometeusobernetes Ingressor Metricizer times out when sending some key information). When "Worker O" spools that job, should it still include the bit_depth attribute?

[fabioperrella]

@julik could you tell me pls where I can find this code in Sqewer?

I mean the part which detects a failure and enqueue the job to the dead-letter queue. I couldn't find it.

[linkyndy]

That would be SQS' job. Received messages by clients need to be deleted from the queue. If they are not, and the message is received more times than it is allowed, it goes to the dead-letter queue.

[fabioperrella]

From what I understand, the job wouldn't be deleted from the queue and it would still with the same attributes (having the bit_depth set), isn't it?

And I can't see a problem with this, but I might be wrong 🤔

[fabioperrella]

If we don't agree on changing this, it's also possible to improve the README giving more details about this process of adding a new attribute.

For example, we can suggest creating another worker and also reviewing the PR using the diff command locally.