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

Issues with multiple posts (without cache - but would be evidenced on a cold start) #770

Closed
timdiggins opened this issue Nov 20, 2018 · 3 comments
Labels
Milestone

Comments

@timdiggins
Copy link
Collaborator

I'm just recording where I'm having some issues with upgrading a mainapp's use of thredded from 0.14.2 to 0.16.x. Mostly thinking aloud, but may turn into something useful.

I get "ActionView::Template::Error: could not obtain a connection from the pool within 5.000 seconds" errors indicating the connection pool isn't large enough when trying to render posts in parallel (ie. #696). So first of all I increase the DB_POOL to accomodate this (25 per worker per max_threads, although in my config workers don't share connection pool so 25 * max_threads should do it).

Although this fixes it, I'm now digger a bit deeper -- why do I need to ever access the db to render a post -- shouldn't everything be prefetched? So it turns out that the culprit is the AtNotificationExtractor. whose users_provider does a fetch of users per post. This isn't very performant. Especially in the case where the number of users is pretty small (less than 100) - I might be better off prefetching the lot. Alternatively I wonder if we should be extracting all the users, then prefetching them as a lump, before splitting into parallel.

So it seems to me that we might want to address some of this:

  1. document the (potential) need to have higher DB_POOL when using parallel.
  2. document the alternative (which is possibly to change render_threads to 1 and drop the onebox filter (my pathway -- I'm not overenamoured of onebox, don't use it that much and don't have fragment caching enabled)
  3. maybe provide a means to provide an aggregated user provider for rendering multiple posts. Will open separate issue for this
@glebm
Copy link
Collaborator

glebm commented Jan 13, 2019

I've just noticed this. Should be fixed by #782.

@glebm glebm added this to the v0.16.4 milestone Jan 13, 2019
@glebm glebm added the bug label Jan 13, 2019
@timdiggins
Copy link
Collaborator Author

@glebm I think on a cold start with a busy server the db connections needed would still near to (assuming puma) CollectionToStringsWithCacheRenderer.render_threads * RAILS_MAX_THREADS (per worker, depending on whether workers share the db connection pool or have their own connection pool). And so we could document that -- but it's a bit exotic, so feels a bit much to put it into the readme. Maybe just having this issue around (closed) might be enough.

@glebm
Copy link
Collaborator

glebm commented Jan 14, 2019

I don't think render_threads affects the necessary connection pool size anymore but I'm not entirely sure (we don't explicitly obtain then release a connection, not sure how it works automatically with threads). It did fix the same issue for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants