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.

Sign up for GitHub

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

Jump to bottom

Add sidekiq docker #443

Merged
merged 6 commits into from Jan 26, 2020
Merged

Add sidekiq docker #443

merged 6 commits into from Jan 26, 2020

Conversation

antodoms
Copy link
Contributor

@antodoms antodoms commented Jan 22, 2020 •
edited

Pull Request Template

Description

  • Add Sidekiq as part of the composer stack (reduce confusion during development)
  • Minor improvements over Gem update without building Dockerfile each time
  • Reduce default redis concurrency to 3

Type of change

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

sojan-official
sojan-official reviewed Jan 23, 2020
View reviewed changes
config/sidekiq.yml
@@ -4,7 +4,7 @@
# pick it up automatically.
---
:verbose: false
:concurrency: 10
Copy link
Member

Choose a reason for hiding this comment

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

any specific reasons why we are making this change ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because sidekiq in docker- compose wasnt running as the rails app used up most of the redis connection pool, so i needed to adjust the concurrency so that both rails ans sidekiq runs fine. I also faces the concurrency issue with heroku with a free redis tier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we increase the connection pool on the Redis used in docker-compose? In Heroku free tier for Redis, the connection limit is 20 as mentioned here https://elements.heroku.com/addons/heroku-redis.

sony-mathew
sony-mathew suggested changes Jan 25, 2020
View reviewed changes
config/sidekiq.yml
@@ -4,7 +4,7 @@
# pick it up automatically.
---
:verbose: false
:concurrency: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we increase the connection pool on the Redis used in docker-compose? In Heroku free tier for Redis, the connection limit is 20 as mentioned here https://elements.heroku.com/addons/heroku-redis.

@sony-mathew
Merge branch 'develop' into pr/443
ea1d154 
# Conflicts:
#	config/initializers/sidekiq.rb
@sony-mathew
Pass sidekiq cofig file
e45ab33 
* passed the env file in base image rather than separately in rails, web packer and sidekiq in docker-compose
* removed un-necessary changes in schema
* changed concurrency to finer values
* removed default size set in sidekiq redis config
* Added the sidekiq config option in Procfile.dev
sony-mathew
sony-mathew approved these changes Jan 26, 2020
View reviewed changes
Copy link
Contributor

@sony-mathew sony-mathew left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sony-mathew sony-mathew merged commit 6325acd into chatwoot:develop Jan 26, 2020
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sojan-official sojan-official sojan-official left review comments

@sony-mathew sony-mathew sony-mathew approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@antodoms @sojan-official @sony-mathew