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

remedy logic, gemfile, gemspec, CI #1

Merged
merged 14 commits into from
Oct 29, 2024
Merged

remedy logic, gemfile, gemspec, CI #1

merged 14 commits into from
Oct 29, 2024

Conversation

Robgra13
Copy link
Contributor

@Robgra13 Robgra13 commented Oct 9, 2024

No description provided.

@Robgra13 Robgra13 requested a review from Azdaroth October 9, 2024 16:55
@Robgra13
Copy link
Contributor Author

Robgra13 commented Oct 9, 2024

Rspec fail is not surprise as spec is designed to fail (I am checking if everything works)

Gemfile Outdated
gem "rubocop-performance"
gem "rubocop-rake"
gem "rubocop-rspec"
gem 'sidekiq'
Copy link
Member

@Azdaroth Azdaroth Oct 10, 2024

Choose a reason for hiding this comment

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

Sidekiq should ideally be a an actual dependency in the gemspec, Gemfile is fo development dependencies

Gemfile.lock Outdated
@@ -0,0 +1,88 @@
PATH
Copy link
Member

Choose a reason for hiding this comment

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

Gemfile.lock for gems should be in .gitignore, odd that it was not there from the beginning

Copy link
Member

Choose a reason for hiding this comment

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

@Robgra13 looks like you haven't deleted it

TODO: Delete this and the text below, and describe your gem

Welcome to your new gem! In this directory, you'll find the files you need to be able to package up your Ruby library into a gem. Put your Ruby code in the file `lib/sidekiq/poison/pill/remedy`. To experiment with that code, run `bin/console` for an interactive prompt.
The Sidekiq Poison Pill Remedy gem enhances Sidekiq's job processing by automatically handling and rescheduling failed jobs (poison pills) with integrated logging and error tracking through Sentry, ultimately improving reliability and performance optimization.
Copy link
Member

Choose a reason for hiding this comment

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

AI or own copywriting? ;P

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 wanted this to be pro so AI 🤖

@@ -0,0 +1,41 @@
# frozen_string_literal: true

require_relative "../lib/remedy/version"
Copy link
Member

Choose a reason for hiding this comment

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

do we need it here?

spec.license = "MIT"
spec.required_ruby_version = ">= 3.0.0"

spec.metadata["allowed_push_host"] = "TODO: Set to your gem server 'https://example.com'"
# spec.metadata["allowed_push_host"] = "TODO: Set to your gem server 'https://example.com'"
Copy link
Member

Choose a reason for hiding this comment

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

to be removed

spec.homepage = "TODO: Put your gem's website or public repo URL here."
spec.summary = "Enhances Sidekiq's job processing by automatically handling and rescheduling failed jobs (poison pills)"
spec.description = "Enhances Sidekiq's job processing by automatically handling and rescheduling failed jobs (poison pills)"
spec.homepage = "https://example.com"
Copy link
Member

Choose a reason for hiding this comment

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

put github repo URL here


spec.metadata["homepage_uri"] = spec.homepage
spec.metadata["source_code_uri"] = "TODO: Put your gem's public repo URL here."
spec.metadata["changelog_uri"] = "TODO: Put your gem's CHANGELOG.md URL here."
spec.metadata["source_code_uri"] = "https://example.com"
Copy link
Member

Choose a reason for hiding this comment

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

put github repo URL here

spec.metadata["source_code_uri"] = "TODO: Put your gem's public repo URL here."
spec.metadata["changelog_uri"] = "TODO: Put your gem's CHANGELOG.md URL here."
spec.metadata["source_code_uri"] = "https://example.com"
spec.metadata["changelog_uri"] = "https://example.com"
Copy link
Member

Choose a reason for hiding this comment

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

put github repo URL here, leading to Changelog, should be created by default

@@ -1,10 +1,10 @@
# frozen_string_literal: true

require "sidekiq/poison/pill/remedy"
require_relative '../lib/sidekiq_poison_pill_remedy'
Copy link
Member

Choose a reason for hiding this comment

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

it should be at most required sidekiq-poison-pill-remedy, require_relative is rarely ever needed for basic things in gems

@Robgra13
Copy link
Contributor Author

Robgra13 commented Oct 23, 2024

Job not going to the DeadSet is the problem of testing environment? As you can see I even did puts "1" and so on to check where is exactly is the problem and it points out that failed job is not going to the DeadSet queue. I did this commit to show what I am working on still.
Also difference I have localy is I have Jobs in Queue: 1

Copy link
Member

@Azdaroth Azdaroth left a comment

Choose a reason for hiding this comment

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

@Robgra13 check if raising an error from the job will help blow it up and have the job available in DeadSet

.rubocop.yml Outdated
@@ -0,0 +1,8 @@
Style/Documentation:
Copy link
Member

Choose a reason for hiding this comment

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

@Robgra13 Best to reuse some existing template to have consistency across projects: https://github.com/BookingSync/rails-transactional-outbox/blob/master/.rubocop.yml

Copy link
Member

Choose a reason for hiding this comment

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

Just remember to remove the excluded files which don't exist in this gem ;)

Gemfile.lock Outdated
@@ -0,0 +1,88 @@
PATH
Copy link
Member

Choose a reason for hiding this comment

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

@Robgra13 looks like you haven't deleted it

README.md Show resolved Hide resolved

def perform(arg)
if arg == 'fail'
Sidekiq.logger.error('Forced failure for testing')
Copy link
Member

Choose a reason for hiding this comment

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

But that will actually be a successful job, no? You would need to raise an error to blow it up

@Robgra13
Copy link
Contributor Author

So I raised an error and failed job is not hiting the DeadSet. I tried different scenarios of the spec for Testing::fake!, inline! and disable!
I also created job loader to check if it will work with running redis server and sidekiq

require "sidekiq/testing"
require "rspec-sidekiq"
require "sidekiq-poison-pill-remedy"
require_relative "jobs/my_job"
Copy link
Member

Choose a reason for hiding this comment

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

given that this is only for testing, please don't include this on a gem level, but rather on a spec_helper level

allow(Sentry).to receive(:capture_message)
end

it "moves job to poison_pill queue and logs message" do
Copy link
Member

Choose a reason for hiding this comment

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

to test this scenario, it would be enough to have any kind of job that could be scheduled by job = MyJob.peform_async(nil) and verify side effects

job = MyJob.peform_async(nil)

# assert number of jobs in default queue
# assert number of jobs in poison_pill queue

SidekiqPoisonPillRemedy.remedy.call(job)

# assert number of jobs in default queue
# assert number of jobs in poison_pill queue

the side effect should be that in default queue the number of jobs will go from 1 to 0, and for poison pill from 0 to 1.

don't use inline mode, use a test one, to make sure the job will not be processed

README.md Show resolved Hide resolved
lib/jobs/my_job.rb Outdated Show resolved Hide resolved
Copy link
Member

@Azdaroth Azdaroth left a comment

Choose a reason for hiding this comment

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

@Robgra13 One last item to address and we are good for the release 💪


allow_any_instance_of(Sidekiq::DeadSet).to receive(:find_job).with(enqueue_job).and_return(job)
allow(Sentry).to receive(:capture_message).and_call_original
allow(Sidekiq.logger).to receive(:fatal)
Copy link
Member

Choose a reason for hiding this comment

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

@Robgra13 please always use and_call_original if you use it for the purpose of spying (i.e. using have_received later)

Copy link
Member

@Azdaroth Azdaroth left a comment

Choose a reason for hiding this comment

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

💪 🐉 Opa!

@Azdaroth Azdaroth merged commit c6f27b0 into master Oct 29, 2024
8 checks passed
@Azdaroth Azdaroth deleted the gem-edit branch October 29, 2024 07:49
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