Suggestions on how to access DB

[rexposadas]

Hi Jeff,

The disclaimer: All this is subjective... feel free to reject all suggestions. Also, I am not yet using revmo. I wanted to see if my approach is even good.

1) I was thinking of steamlining the code a bit by providing the handler to the DB a different way.

Please check out:

Post DB

I made up the post struct (it's not going to the pull request)

Suggestions:

1) Access a global handles to the DB.

This:

    func (article *Article) All(s *mgo.Session) []*Article {
        articles := []*Article{}
        coll := article.Collection(s)
        coll.Find(nil).Sort("-Published").Limit(10)

or this:

    func (p *Post) SetAuthor(author string) {  // no mgo session in param
        bucket, dberr := db.Bucket()
        bucket.Post.Update(  // access a collection via a global handler

2) make use of defer. I think this is clean way of closing our connections.

defer bucket.Close() // close connection when we leave scope

3) HIGHLY subjective

    func (article *Article) All(s *mgo.Session) []*Article {
    func (p *Post) SetAuthor(author string) {  // no mgo session in param

I prefer using a single character to indicate the pointer in the func signature. revel does it the same way.

That's about it. I'm still looking through the code. I wonder if we should seed the blog with data.

I would love to hear your thoughts. Again, feel free to reject all suggestions.

Thanks, Rex

[jgraham909]

Hi Rex, we should discuss this and sort out the best approach. I'm open to your ideas, but I also want to provide justification for whatever approach we go with so that we can document the design philosophy so that it is easier for others to grok once (if) it gets merged into revel.

1. I'm not too much of a fan of passing the session in the model either, but I do think attaching the connection/session to the controller provides some benefit. Given how mgo works and that various applications may want a consistent data model across a single request attaching to the controller provides the following benefits;
   1. No extraneous spawning new connections. Each controller is its own go routine, only if a controller spawns internal go routines would there be a need to open more connections, and the controller implementer would know this. This minimizes some overhead (network latency, memory, etc.) and minimizes GC.
   2. Each controller has control over what type of data consistency across a single request. If none is needed it may be beneficial to fan-out on the db connections using go routines or a single connection may suffice.
2. If we refactor like you say then defer is the obvious approach and would be a necessary part. I like that your approach is somewhat more idiomatic go, tightens the scope so that it is obvious what is available and in scope. My main concern though is how much garbage is created on each request. Let's say a single request loads 10 recent posts, and 3 specific users. Following your proposed model that would create and close 4 db connections (1 for the summary 10 post view, and one find per specific user). Whereas if we attach the collection to the controller we only create one connection for the life of the current request, again assuming no spawned go routines by the controller. I'm not set on this but I am leaning towards the single connection per controller.
3. I'm fine adjusting the pointer names to single character. This follows revel convention, and from what I can tell is consistent with go convention too.

Regarding the collection bucket, I like centralizing the collections used. Perhaps we could move the collection names to app/conf? I'm thinking of a scenario (in the future) where revel is wildly successful or someone is creating their second or third revel app and re-uses a module, either third party or internal. In this scenario it would be preferable for the app integrator to dictate what collections are used so your approach is great. However, it would not be great to require editing code. If we moved the config to app/conf we get to defer collection name to the application creator, and avoid the need to edit db connector pooling code as features are added or dropped during app development. Regardless of which connection approach we go with I think centralizing the collection name in app/conf makes sense to avoid either editing the db code or the model code, this should make code re-use via modules a bit more streamlined.

"I wonder if we should seed the blog with data." I think that's a great idea especially given this is a sample app. Having isolated code that fires off on app start would be great.

Again like I indicated above I'm open to discussing your approaches and willing to be convinced otherwise.

[rexposadas]

Hey Jeff,

Sorry for the delayed response. Work gets in the way of fun sometimes =)

I was so concerned about making the code "Go Like" that I neglected the DB hits. But I get your point. Let's go with what you have.

"Each controller is its own go routine" -> I wasn't aware of that. That's a big change in my thinking.

Great, we both agree on seeding the blog with data.

[jgraham909]

No worries I've been a bit slow over here too. Summer time in Portland makes up for the 9 months of rain so priorities shift a bit here in the summer. ;)

Yeah I was hoping to come up with a more "go like" approach, but I think the current approach is the appropriate balance of trade-offs.

I think the actual details are that each incoming HTTP request creates a new goroutine and since that HTTP request is exposed to revel apps via the revel controller we can effectively think of each controller instance as its own go routine and thus data that should only be available to the current request really should be attached to the controller.

[rexposadas]

Sorry it's been slow on my side. I got CRUD working for Articles and got to know the code better. Do you prefer getting small changes in pull-requests or one big chunk?

[jgraham909]

As long as the changes are not "philosophically" different, big chunks are fine. Divergent implementations are better as small proofs of concept first and appropriate discussion.

Things have been slow on my end too so no worries. On Jun 24, 2013 11:55 PM, "rex posadas" notifications@github.com wrote:

Sorry it's been slow on my side. I got CRUD working for Articles and got to know the code better. Do you prefer getting small changes in pull-requests or one big chunk?

— Reply to this email directly or view it on GitHubhttps://github.com/jgraham909/bloggo/issues/2#issuecomment-19956626 .

[rexposadas]

Hi Jeff, were you able to run the test suite for this app?

bin/revel test github.com/jgraham909/bloggo

results in 0 tests ran. I added the testrunner module in app.conf and verified that there are tests in /tests.

This might not be specific to your app, it could be a revel framework issue.

[rexposadas]

Ah, nvm. I had to add the following line to conf/routes

module:testrunner

[jgraham909]

Please checkout 81559716800d7f6d3ba25e24116c2dee9a98f791 I refactored to make the db configurable. As part of that there is now an app.AppInit this seems like the logical choice for database seed data. Let me know if that won't work for what you are thinking.

[jgraham909]

Closing this out as I think most issues are addressed or will need more detailed/targetted follow up.