Skip to content

Commit

Permalink
Refactor recent client changes
Browse files Browse the repository at this point in the history
I find `push` is easier to read this way. push_bulk is quite complex and adds a net-negative layer of abstraction.
  • Loading branch information
mperham committed Mar 14, 2022
1 parent 44a5bb3 commit ba557da
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 19 deletions.
24 changes: 12 additions & 12 deletions lib/sidekiq/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,14 @@ def initialize(redis_pool = nil)
# push('queue' => 'my_queue', 'class' => MyJob, 'args' => ['foo', 1, :bat => 'bar'])
#
def push(item)
push_bulk(item.merge("args" => [item["args"]])).first
normed = normalize_item(item)
payload = middleware.invoke(normed["class"], normed, normed["queue"], @redis_pool) do
normed
end
if payload
raw_push([payload])
payload["jid"]
end
end

##
Expand Down Expand Up @@ -100,10 +107,11 @@ def push_bulk(items)

normed = normalize_item(items)
payloads = args.map.with_index { |job_args, index|
copy = normed.merge("args" => job_args, "jid" => jid || generate_jid)
copy = normed.merge("args" => job_args, "jid" => SecureRandom.hex(12))
copy["at"] = (at.is_a?(Array) ? at[index] : at) if at

result = process_single(items["class"], copy)
result = middleware.invoke(copy["class"], copy, copy["queue"], @redis_pool) do
copy
end
result || nil
}.compact

Expand Down Expand Up @@ -225,13 +233,5 @@ def atomic_push(conn, payloads)
conn.lpush("queue:#{queue}", to_push)
end
end

def process_single(job_class, item)
queue = item["queue"]

middleware.invoke(job_class, item, queue, @redis_pool) do
item
end
end
end
end
6 changes: 1 addition & 5 deletions lib/sidekiq/job_util.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ def normalize_item(item)

raise(ArgumentError, "Job must include a valid queue name") if item["queue"].nil? || item["queue"] == ""

item["jid"] ||= SecureRandom.hex(12)
item["class"] = item["class"].to_s
item["queue"] = item["queue"].to_s
item["created_at"] ||= Time.now.to_f

item
end

Expand All @@ -61,9 +61,5 @@ def normalized_hash(item_class)
def json_safe?(item)
JSON.parse(JSON.dump(item["args"])) == item["args"]
end

def generate_jid
SecureRandom.hex(12)
end

This comment has been minimized.

Copy link
@adamniedzielski

adamniedzielski Mar 17, 2022

Contributor

This is the change that I can't really understand. generate_jid explains what the method does. You're going back to duplication. It's just in two places, but every time I see SecureRandom.hex(12) my brain has to pause for a second to think about the purpose of the code.

end
end
3 changes: 1 addition & 2 deletions lib/sidekiq/worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ def perform_inline(*args)

# validate and normalize payload
item = normalize_item(raw)
item["jid"] ||= generate_jid
queue = item["queue"]

# run client-side middleware
Expand Down Expand Up @@ -355,7 +354,7 @@ def sidekiq_options(opts = {})

def client_push(item) # :nodoc:
raise ArgumentError, "Job payloads should contain no Symbols: #{item}" if item.any? { |k, v| k.is_a?(::Symbol) }
build_client.push_bulk(item.merge("args" => [item["args"]])).first
build_client.push(item)
end

def build_client # :nodoc:
Expand Down

2 comments on commit ba557da

@adamniedzielski
Copy link
Contributor

Choose a reason for hiding this comment

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

😢 🤷‍♂️

@mperham
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know, I'm unhappy any time I have to roll back changes which someone kindly contributed but my #1 concern for the code is always my own ability to understand and maintain it long-term. It's quite common for me to rewrite code that I get into my "style" so that I can better understand it and read it going forward.

Regarding generate_jid, there's no reason from my POV to extract this into its own tiny method. We're not going to change how JIDs are generated, I don't want to make it easy for people to change it (i.e. by extracting a method you allow others to easily monkeypatch it), it does add a tiny bit of runtime overhead and the impl itself is so straightforward that it doesn't hide any complexity. DRY is a concern but two copies isn't enough to justify.

Contrary to what was "idiomatic" Ruby a decade ago, I prefer classes which don't have lots of tiny extracted methods. I prefer a few big methods where I can "see" everything that is going to happen, rather than lots of jumping around between dozens of methods. I find it keeps the cognitive overhead low. S::Processor#process is a good example, it encapsulates the steps necessary to process a job, with just one significant extracted method dispatch (execute_job is extracted for testing).

https://github.com/mperham/sidekiq/blob/main/lib/sidekiq/processor.rb#L145

Please sign in to comment.