Custom slick thread pool, plus some async db cleanup #424
Conversation
|
The wheel chooseth @scottfrazer and @mcovarr, but happy for anyone else to take a look. Am leaving it as two commits as the first has the more important change, and the second is verbose cleanup. |
| // Update the database state: | ||
| val newBackendInfo = JesCallBackendInfo(Option(JesId(runId)), Option(JesStatus(currentStatus.toString))) | ||
| globalDataAccess.updateExecutionBackendInfo(workflowId, BackendCallKey(call, pipeline.key.index), newBackendInfo) | ||
| globalDataAccess.updateExecutionBackendInfo( | ||
| workflowId, BackendCallKey(call, pipeline.key.index), newBackendInfo)(ExecutionContext.global) |
There was a problem hiding this comment.
I remember discussing this at the time it was written though it seems we kinda forgot to come back and clean it up. 😦
There was a problem hiding this comment.
This will almost certainly run afoul of the incoming intel changes, but that makes it @mcovarr's problem :)
There was a problem hiding this comment.
oh definitely it's already conflicting...
|
I'm certainly not a fan of littering a code base with FIXMEs but in this case I do think it's warranted in that one spot (you know where I mean 😄) |
|
|
||
| object DataAccess { | ||
| val globalDataAccess: DataAccess = new slick.SlickDataAccess() | ||
| } | ||
|
|
||
| trait DataAccess { | ||
| trait DataAccess extends AutoCloseable { |
There was a problem hiding this comment.
I haven't really seen anyone use this in scala, presumably just because it's a new-ish construct. I'm not saying this to poo poo it (it seems like a good idea), but more just to ensure we understand how it's actually working here. Is this something you've seen other slick stuff doing?
There was a problem hiding this comment.
I have not seen other slick code using this, but then again-- I haven't seen most typesafe code (slick, spray, akka, etc.) tidying up. Usually one has to go digging deep into the test code / google groups for finding instructions on how to cleanly shutdown.
What happened here was that I was wiring in the ExecutionContexts, when I realized I already made SlickDataAccess extend AutoCloseable back in PRs #371 / #403. The change was made such that when the liquibase/slick comparison finished, the both databases would be .autoClosed using better.files' ARM.
There was a problem hiding this comment.
Yeah ARM & Scala has been a weird thing for a while now :)
As long as you're reasonably sure that it's doing what you think it's doing (or at least not doing anything unfriendly) I'm all for it - as I said, on paper it seems like a good idea. I was just a bit leery about it being fairly unvetted in the scala/slick context.
There was a problem hiding this comment.
Man I'm so confused :) I've never seen an AutoCloseable before (I'm guessing it's this). I read the API but I guess I'm still not clear on why it was added!
|
It's entirely possible that this resolves all of the big bang problems, including the spray responsiveness. From what I saw yesterday the pain and suffering was primarily coming from the DB access. |
kshakir
force-pushed
the
ks_custom_slick_thread_pool
branch
from
cf0616b
to
abc414b
Compare
|
Added a basic regression test that verifies our deadlock workaround. |
kshakir
force-pushed
the
ks_custom_slick_thread_pool
branch
from
4b91db3
to
4a7d46e
Compare
|
👍 nice job getting this to work, though I really hope Slick 3.2.0 makes this unnecessary. 😄 |
|
At some point over the weekend I'll see if it seems to resolve the other issues we were seeing as well. If so I'll go ahead and close the ticket I have. I don't think we really need to bulkhead out anything other than the DB IO at the moment, will take a look. |
|
@kshakir I want to tech talk on this a bit prior to merging. I think it's fine as-is but want to make sure we think through the nuance and/or logical next steps |
kshakir
force-pushed
the
ks_custom_slick_thread_pool
branch
4 times, most recently
from
ae3c19a
to
779a225
Compare
|
@kshakir I wonder if there's a way to put in a little check in all of our funcs here which verifies that the EC which comes in is the one that we're hoping it to be? As I mentioned at tech talk I always get sketched out at these implicit ECs. Either way, 👍 from me |
|
👍 |
kshakir
force-pushed
the
ks_custom_slick_thread_pool
branch
2 times, most recently
from
c193390
to
2fe3131
Compare
Shutting down our custom slick thread pool when the database closes. Added changelog item. Added deadlock test. Passing in the execution context used for flatmap'ing slick futures, and a) NOT using Slick's internal thread pool, and b) not just defaulting to the global execution context (...mostly). - http://slick.typesafe.com/doc/3.1.0/gettingstarted.html#quick-introduction - https://github.com/alexandru/scala-best-practices/blob/master/sections/4-concurrency-parallelism.md#411-must-not-hardcode-the-thread-pool--execution-context - https://groups.google.com/d/msg/scalaquery/5i543a9Vkt0/7ENpRx_35sYJ
kshakir
force-pushed
the
ks_custom_slick_thread_pool
branch
from
2fe3131
to
40da8b8
Compare
Custom slick thread pool, plus some async db cleanup
No description provided.