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

Clarify database connections and recurring processes in README.md #1015

Merged
merged 5 commits into from
Jul 27, 2023
Merged

Clarify database connections and recurring processes in README.md #1015

merged 5 commits into from
Jul 27, 2023

Conversation

blumhardts
Copy link
Contributor

See #1013 for more context.

Comment on lines -62 to +63
- [Production setup](#production-setup)
- [Queue performance with Queue Select Limit](#queue-performance-with-queue-select-limit)
- [Production setup](#production-setup)
- [Queue performance with Queue Select Limit](#queue-performance-with-queue-select-limit)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These don't seem to make sense under Database connections, so I moved them up a level.

@@ -954,24 +954,51 @@ Keep in mind, queue operations and management is an advanced discipline. This st

### Database connections

Each GoodJob execution thread requires its own database connection that is automatically checked out from Rails’ connection pool. For example:
GoodJob job executor processes require the following database connections:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"job executor process" is maybe just a fancy way to say "worker", but I saw "executor" a couple other times in the README and wasn't sure what the correct language for GoodJob was, so I just went with it.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm trying to avoid using "worker" because it can be ambiguous: sidekiq/sidekiq#4971 (comment)

The meaning here is "any process where GoodJob is executing jobs (eg the executable or async-mode)".

I agree with you that the current wording is clunky and not necessarily clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"job executor process" is kind of clunky, but it is at least less ambiguous than "worker," in my opinion. Mike Perham also refers to the same concept as a "process" in the Sidekiq docs, but using that word alone is again ambiguous (a web server that is enqueuing jobs but not executing them is also running as a process), so sticking a qualifier on the front like "job executor" hopefully disambiguates the term.

README.md Outdated
Each GoodJob execution thread requires its own database connection that is automatically checked out from Rails’ connection pool. For example:
GoodJob job executor processes require the following database connections:

- 1 connection for the job listener, a.k.a. `LISTEN/NOTIFY`.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I understand it, LISTEN/NOTIFY is used by the worker process to pull enqueued jobs from the database. Is this also used to enqueue jobs in the database? Should this connection also be accounted for in the web server process that enqueues jobs?

Copy link
Owner

Choose a reason for hiding this comment

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

Only the LISTEN part runs in the GoodJob processes that execute jobs (cli or async). LISTEN requires its own dedicated database connection which is used in a longrunning background thread.

The NOTIFY part is a short query that is emitted when a job is enqueued and uses the same database connection that is used to enqueue the job in the same context (eg in a controller in a puma thread).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thank you for the clarification! With that in mind, I have removed the ", a.k.a. LISTEN/NOTIFY" portion from this line as I think it makes things less clear, not more. I have also updated the wording from "1 connection for the job listener" to "1 connection for the job reader" as I now see you have used the phrasing "read jobs" elsewhere in the docs and I'd like to remain consistent.

I debated adding your above comment basically verbatim into the docs as an "Enqueueing and reading jobs" subheader under this "Database connections" section. I think it makes the connection usage and requirements clearer, but might be letting the implementation details leak into the docs a little too much.

@@ -954,24 +954,51 @@ Keep in mind, queue operations and management is an advanced discipline. This st

### Database connections

Each GoodJob execution thread requires its own database connection that is automatically checked out from Rails’ connection pool. For example:
GoodJob job executor processes require the following database connections:
Copy link
Owner

Choose a reason for hiding this comment

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

I can think the extra detail in this section overall is useful.

And I think the level of detail implies something different than my general advice:

Be generous with your database connection pool size. Better to configure a slightly larger pool than you think you need (which will only consume as many connections as it actually needs) than a smaller one (which will lead to connection pool timeouts and exceptions).

Copy link
Contributor Author

@blumhardts blumhardts Jul 20, 2023

Choose a reason for hiding this comment

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

Be generous with your database connection pool size. Better to configure a slightly larger pool than you think you need (which will only consume as many connections as it actually needs) than a smaller one (which will lead to connection pool timeouts and exceptions).

I think this is good advice. My immediate follow-up question to that as a user of the gem is, "okay, well, then what's the lower bound where I'll start experiencing timeouts and exceptions, so I can make sure I always avoid that?"

Maybe that could be answered more concisely than what I am currently proposing.

The original docs answered that for :async mode, but lost me when I started thinking about :external mode. I think it was the lack of clarity between what a "job executor" process and its threads need and what a "job enqueuer" process and its threads need. As it turns out, the executor needs all the extra connections and the enqueuer doesn't need any. Because those are the same process in :async mode (albeit with different thread pools for serving web requests and performing jobs), the requirements get jumbled together.

@bensheldon
Copy link
Owner

Thank you for this! Feel free to keep tweaking and I'll also make a few tweaks and merge it 💖

@bensheldon bensheldon merged commit 73b3cf3 into bensheldon:main Jul 27, 2023
@bensheldon bensheldon added the documentation Improvements or additions to documentation label Jul 27, 2023
@blumhardts blumhardts deleted the patch-1 branch July 27, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants