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

first version of play-slick #1230

Closed
wants to merge 1 commit into from

Conversation

@freekh
Copy link
Contributor

commented Jun 20, 2013

No description provided.

@huntc

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2013

Wow, this looks very interesting. I'm thinking that we should deprecate anorm in the same fowl swoop. Will talk with the rest of the team and review this. Thanks @freekh!

@freekh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2013

Great :)
I just wanted to send in the PR - there might be some revisions that should be done especially on the documentation side, but that is fine with me :)

F

On Jun 20, 2013, at 3:02 PM, Christopher Hunt wrote:

Wow, this looks very interesting. I'm thinking that we should deprecate anorm in the same fowl swoop. Will talk with the rest of the team and review this. Thanks @freekh!


Reply to this email directly or view it on GitHub.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 20, 2013

play2-master-PRs #252 FAILURE
Looks like there's a problem with this pull request

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

What is the best strategy to integrate developments done after this pull request in the play-slick repo?
Should we rebuild the PR or send separated pull requests?

@freekh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2013

I am not sure. I guess it is easier for the play team if we were to rebuild the PR. By rebuilding the PR I mean adding commits to the current branch. Good thing you reminded me to get this build issue fixed :)

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

Ok, I agree, having a single PR should be easier.
I think the plugin is stable enough now to do it (including the execution context stuffs)

@huntc

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

I think just keep on commiting to the branch associated with the PR. @freekh should allow you access to his forked repo if he hasn't already. Thanks!

@freekh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2013

@loicdescotte added you as a collaborator on my Play20 fork now

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

Thanks :)
I guess I should merge everything in a single commit to ease the merge

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

Should be OK now
I've also update the sample and removed those inside the framework/src/play-slick folder

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 24, 2013

play2-master-PRs #258 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 24, 2013

play2-master-PRs #259 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 24, 2013

play2-master-PRs #260 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 24, 2013

play2-master-PRs #261 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Jun 24, 2013

play2-master-PRs #262 SUCCESS
This pull request looks good

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2013

Build Successful, Finally working :) (sorry for the jenkins flood)
Fixed some dependencies in the sbt build + ./build scalariform-format for indentation errors

@freekh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 24, 2013

Awesome Loic! You rock!

@@ -0,0 +1,28 @@
## Using a separated execution context

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

The file naming convention that we've been using for our documentation is upper camel case. In addition, since each file name needs to be unique within the entire documentation (not just in a directory), there needs to be some sort of namespacing. So this filename won't work. I think ScalaSlickExecutionContext would be a better name here.

@@ -0,0 +1,33 @@

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

Filename should follow the naming conventions, eg ScalaSlickTables.md.

@@ -0,0 +1,114 @@
## Multiple datasources and drivers

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

Filename should follow naming conventions, eg ScalaSlickDrivers.md


This is an example usage:

```scala

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

Code samples should be in separate files, compiled and tested, as per:

https://github.com/playframework/Play20/blob/master/documentation/manual/hacking/Documentation.md

@@ -192,6 +192,10 @@ object PlayBuild extends Build {
}
).dependsOn(PlayJavaJdbcProject)

lazy val SlickProject = PlayRuntimeProject("Play-Slick", "play-slick")
.settings(libraryDependencies := slickDependencies)
.dependsOn(PlayJdbcProject, PlayJavaProject, PlayTestProject % "test")

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

Does it really need PlayJavaProject? It shouldn't.

This comment has been minimized.

/**
* Helper object to access Databases using Slick
*/
object DB extends DB {

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

I'm just wondering if having the same name as what PlayJdbc provides might cause confusion.

import akka.actor.ActorSystem

object SlickExecutionContext {
val executionContext = ActorSystem("slick-plugin-system").dispatchers.lookup("slick.execution-context")

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

A new actor system should not be created here. It's going to use the same configuration as the Play one, which means if you turn on remoting in the Play one, it will turn it on here too, and both will try to listen on the same port, and Play will blow up. Also, this actor system never gets shutdown, so there's a resource leak here.

Instead, use the Play AkkaPlugin to get an actor system to get an execution context.

import play.api.Play.current
import play.api.db.slick._

trait SlickController { self: Controller =>

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

Play 2.2 provides an ActionBuilder trait that allows you to define an execution context for running actions. Use that, and this class will just be 3 lines of code.

This comment has been minimized.

Copy link
@loicdescotte

loicdescotte Jun 27, 2013

Contributor

Should be better this way

object SlickAction extends ActionBuilder[Request] {
  def invokeBlock[A](request: Request[A], block: (Request[A]) => Future[SimpleResult]) = block(request)
  override def executionContext: ExecutionContext = SlickExecutionContext.executionContext
}
Then you can configure the slick thread pool in your application.conf file :

```
slick {

This comment has been minimized.

Copy link
@jroper

jroper Jun 24, 2013

Member

Does this work? It doesn't look right to me. Please write tests to ensure that it's being invoked in the right execution context (you can look at the current thread name).

This comment has been minimized.

Copy link
@loicdescotte

loicdescotte Jun 25, 2013

Contributor

Do you have an example of how to retrieve this information? (thread name)
Thanks

This comment has been minimized.

Copy link
@jroper

jroper Jun 25, 2013

Member

Something like Thread.currentThread().getName()

This comment has been minimized.

Copy link
@loicdescotte

loicdescotte Jun 25, 2013

Contributor

Ok yes of course, stupid me I was thinking of a specific Play execution context method :/
Thanks :)

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2013

Thanks for the feedback,
I will try to start working on it this tonight

@freekh

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2013

Thanks @jroper for feedback and thanks to you @loicdescotte for addressing it. Sorry for being unavailable to fix, but I only partially online this week :/

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2013

Hi,

@cvogt has made a very interesting pull request playframework/play-slick#59

As it changes the SlickAction to handle the slick session, maybe we should integrate it instead of using the ActionBuilder?

@nraychaudhuri

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2013

I think we integrate totally integrate that in here.

@cvogt

This comment has been minimized.

Copy link

commented Jun 28, 2013

added another PR to slick-play to make it compatible with ordinary Slick. Before play-slick used it's own DB interface. I replaced it with Slick's in playframework/play-slick#60 I think that's it for now from the Slick team :). In total I submitted 3 PRs, all of which contain source compatibility breaking improvements. So I think it would be good if we get them in before we merge into Play main.

liutaon referenced this pull request in playframework/play-slick Aug 16, 2013
Fredrik Ekholdt
Merge remote-tracking branch 'freekh/2.2' into 2.2
Conflicts:
	samples/play-slick-sample/project/plugins.sbt
@liutaon liutaon referenced this pull request Aug 18, 2013
@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2014

Hi,
If I refer to this #1518 (comment) , if the same logic is applied for slick, I guess we should create a good play-slick activator template rather than integrate it into play-core?
There is a few templates for play-slick but they don't seem usable out of the box for a play-slick beginner (no use of DBAction for example)

@julienrf

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2014

Maybe that’s not the right place to discuss this, but I don’t see the point of DBAction, can you give me some lights? I mean, anyway the HTTP layer has nothing to do with the persistence layer, has it? Or maybe it could if your application is very simple, but as your code grows you want to keep things in different places so that you can independently test and validate them.

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2014

@julienrf Slick uses only blocking IO. The goal of DBAction is to provide a slick execution context with a bigger thread pool to execute the blocking queries. This way anyone can use slick without thinking about execution contexts. But of course DBAction is not mandatory to use play-slick.

@julienrf

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2014

Ok, thanks for clarification.

@loicdescotte

This comment has been minimized.

Copy link
Contributor

commented Apr 15, 2014

Hi all,
I've published a template here https://github.com/loicdescotte/activator-play-slick
Comments or contributions are welcome :)

@freekh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2014

wow .. trop fort loic :) (and nice avatar)

@kev009

This comment has been minimized.

Copy link

commented May 26, 2014

I think this can be closed due to decision to keep it out of core

@cvogt

This comment has been minimized.

Copy link

commented May 26, 2014

@jroper: I can't close this ticket. Please give @szeiger and me the relevant permissions. Thx

@jroper jroper closed this May 26, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.