-
-
Notifications
You must be signed in to change notification settings - Fork 502
Multi-thread DB Queue #1242
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
Multi-thread DB Queue #1242
Conversation
Example |
qaisjp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some good work, thank you!
Review:
- It doesn't build on Windows, so we can't merge this until that works. Don't worry about macOS if you can't get it to work, but you'll save me a little bit of work later if you are able to fix it.
- Left a few minor comments
|
|
||
| shared.m_Mutex.Lock(); | ||
|
|
||
| // Log to console if connection count is creeping up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been removed and not reimplemented in the new system. I assume this is intended as it's no longer a bottleneck.
Is there other dead code or no-longer-useful stuff you can rip out too, such as m_uiConnectionCountWarnThresh?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pentaflops what about this one? (and the comment by @Citizen01)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The message is to help scripters who inadvertanly call dbConnect multiple times. I think it should not be removed
qaisjp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see #1242 (comment)
This reverts commit 6c3be11.
|
This PR was reverted, but after studying the current source code I've found that this issue was kind of already addressed since this commit 58e993d which added a queue option for Btw changing the order of parameters in the host string can probably also do the trick, because the host string is used by default as the queue name. But this is probably not a good idea to rely on something like that, it's better to customize the queue name manually. |
Problem
After transferring the databases to Amazon, we were faced with the problem of a long DB response. Research shown that request queue is quickly filled, the reason is query response to Amazon database can take 2 seconds or more. The current DatabaseManager creates a new queue using the database connection string, and not for a connection element. Thus, a single queue will be created for one database, processing requests many connections in one thread.
Solution
The logic of DatabaseManager and DatabaseJobQueue has been changed. Each individual connection is processed in a separate thread and doesn't block the processing of requests from other connections. The dbGetConnectionQueueSize function was also added, usage example is shown below.
New function
Returns
falseon failure.This change has been tested and successfully operated on our project.
With love from NEXTRP Team