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

Feature/note feature transfer #17

Merged
merged 19 commits into from
May 2, 2019
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@

# Output of the go coverage tool, specifically when used with LiteIDE
*.out

# Editor folders
.vscode
118 changes: 118 additions & 0 deletions connectionmanager/connectionmanager.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
package connectionmanager
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connectionmanager feels like an eyesore to me, suggest package connection if it doesn't conflict with anything else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've fought a lot with this name. Happy for something better!

Connection isn't quite accurate since this manages the initialization/shutdown of any number of items as long as they support the initializer/closer interfaces. Indeed, connection is probably even wrong, though it nods to the intent of this managing things like database/elastic search connections. It provides a method of bringing them up and shutting them down as a group, rather than individually. I don't have a good name, so I stuck with connection manager. The file used to be called connectionmgr, but Levi set me straight on short names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

connectiongroup? Naming is hard, I'll come back to it if I can think of something...


import (
"log"
"sync"
)

// Initializer is the interface that initializes a connection of some kind.
type Initializer interface {
Initialize()
}

// Closer is the interface that gracefully closes a connection of some kind.
type Closer interface {
Close()
}

// InitializerCloser is the interface the both initializes and gracefully closes a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that both

// connection of some kind.
type InitializerCloser interface {
Initializer
Closer
}

// CloseFunc is a function that gracefully shuts down a connection as a side effect.
type CloseFunc func()

// ConnectionManager allows multiple items needing initialization or shutdown to be
// managed as a group.
//
// Initialization and Close of connections are managed independently of each other. Once
// created the connection manager can accept any number of items supporting initialization,
// close, or both. The ManageInitialization, ManageClose, and ManageConnection methods can
// be called as many times as needed in any order to add managed items. They are variadic
// functions, so multiple items can be added in a single call.
//
// Initialization items will immediately start initialization in a separate go routine
// once the item is added to the ConnectionManager. An internally managed sync.WaitGroup
// is made available. Calling WG.Wait() on the ConnectionManager will block the current
// go routine until all initialization functions are complete.
//
// Closers are queued up internally running only when the ConnectionManager's Close method
// is called. The ConnectionManager runs each Close method in a separate go routine and blocks
// blocks until all are complete.
type ConnectionManager struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe a more idiomatic naming scheme would be to remove Connection prefix from the names within this package to avoid the stutter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manager feels too generic to me as a name - but again, I can't seem to name this functionality. Help!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manager as a whole is generic but it's within the context of connectionmanager: the consumer of the package will be typing connectionmanager.ConnectionManager repeatedly. But it could be connectionmanager.Manager or even connectiongroup.Manager

closerChan chan CloseFunc
Close CloseFunc
WG sync.WaitGroup
}

// New initializes a new ConnectionManager object that can be used
// to manage the life of long lived remote connections such as to a database.
func New(logger *log.Logger) ConnectionManager {
closerChan := make(chan CloseFunc)
shutdown := make(chan struct{})
var stopwg sync.WaitGroup
go func() {
closers := []CloseFunc{}
loop:
for {
select {
case <-shutdown:
logger.Println("Shutting Down")
break loop
case c := <-closerChan:
stopwg.Add(1)
closers = append(closers, c)
}
}

for _, c := range closers {
go func(c func()) {
c()
stopwg.Done()
}(c)
}
}()
return ConnectionManager{
closerChan: closerChan,
Close: func() {
shutdown <- struct{}{}
stopwg.Wait()
},
}
}

// ManageInitialization allows the connection manager to accept any number of items
// matching the Initializer interface and initializes each in parallel. The wait
// group is managed to allow callers to block until all managed initialization
// methods are complete.
func (cm *ConnectionManager) ManageInitialization(initializers ...Initializer) {
for _, initializer := range initializers {
cm.WG.Add(1)
go func(i Initializer) {
i.Initialize()
cm.WG.Done()
}(initializer)
}
}

// ManageClose allow the connection manager to accept any number of items matching
// the Closer interface. It queues them up internally. When Close is called on
// the connection manager, all queued Close methods are executed in parallel.
// The close method blocks until managed Closers are complete.
func (cm *ConnectionManager) ManageClose(closers ...Closer) {
for _, closer := range closers {
cm.closerChan <- closer.Close
}
}

// ManageConnection accepts any number of items matching the InitializerCloser
// interface and manages both an item's initialization and close.
func (cm *ConnectionManager) ManageConnection(initializerClosers ...InitializerCloser) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ManageConnection seems to restart a connection, close -> initialize -> return. Would a more accurate name be Restart?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't...

Though the ordering is a bit confusing, this queues up the close method of the managed item first (which will not run until the connection manager itself has the Close method called. Once queued, it then adds the items initialization function, which immediately starts a go routine that initializes the item. The order ensure the close function is managed before initialization begins.

The fact that you are asking means this is just a bit too confusing. I'll add additional comments to clarify.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I understand, maybe EnqueueConnection/EnqueueClosers/EnqueueInitializers more accurately describes what they're doing?

for _, ic := range initializerClosers {
cm.ManageClose(ic)
cm.ManageInitialization(ic)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file looks like beautiful go code :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow, high praise! Thanks!

62 changes: 47 additions & 15 deletions database/db.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
package database

import (
"fmt"
"github.com/go-pg/pg"
"github.com/robinjoseph08/go-pg-migrations"
"log"
"os"
"time"

"github.com/go-pg/pg"
migrations "github.com/robinjoseph08/go-pg-migrations"
)

var (
Expand All @@ -15,19 +15,24 @@ var (

// DBConfig wraps config for connecting to a database.
type DBConfig struct {
Address string
User string
Database string
Password string
Address string
User string
Database string
Password string
Logger *log.Logger
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be made a Service Logger at this point? Do we fully standardize on it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a service logger - its at the point now with the last updates Michael made to not have a logger per level and accept human readable log level that its ready for prime time/doesn't have any major usability issues I can see(take that with a grain of salt as I haven't used it in any of the code I've written)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's my proudest go code. That aside, switching between the two is fairly painless and none of the consumer code for the logger will have to change.

EnableLogging bool
}

// DB wraps a client for a database.
type DB struct {
Client *pg.DB
Client *pg.DB
Logger *log.Logger
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be embedded into this struct and not a public field(I can't think of a use case other code would have for needing access to the database's logger), with the additional upside that methods on this struct can make logging statements via db.Println() versus db.Logger.Println
https://golang.org/doc/effective_go.html#embedding
https://travix.io/type-embedding-in-go-ba40dd4264df

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note though that a service logger satisfies the logger interface so can be passed as the logger value here, and that the underlying Postgres client only accepts a logger interface so won't be able to take advantage of any additional methods we have(though our database package can)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know log.Logger is still a concrete type, not an interface :(

golang/go#13182

initializer func(*DB)
}

// Close closes a connection to a database. Once close has been called calling other methods on db will error.
func (db *DB) Close() {
db.Logger.Println("Closing database connection")
db.Client.Close()
}

Expand Down Expand Up @@ -57,18 +62,45 @@ func (d dbLogger) AfterQuery(q *pg.QueryEvent) {
d.logger.Printf("executed query\n%+v ", query)
}

// New returns a new DB object which wraps
// a connection to the database specified in config
// New returns a new DB object which wraps a connection to the database specified in config
func New(config DBConfig) DB {
db := pg.Connect(&pg.Options{
Addr: config.Address,
User: config.User,
Database: config.Database,
Password: config.Password,
})
if os.Getenv("QUERY_DEBUG") == "ON" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tears of joy to see this removed.

db.AddQueryHook(dbLogger{logger: log.New(os.Stdout, fmt.Sprintf("%s: %s: ", config.Address, config.Database), log.LstdFlags)})
if config.EnableLogging {
Copy link
Contributor

@willmichael willmichael May 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to think about how the Service Logger can be integrated into this, so we don't have to specifically enable at the DB level... But everything I can think of is slightly convoluted. Unless we want to restrict the DB logging to a specific level like debug

db.AddQueryHook(dbLogger{logger: config.Logger})
}
return DB{
Client: db,
Logger: config.Logger,
initializer: RunMigrations,
}
}

// Initialize runs any needed set up operations for the database. This defaults
// to RunMigrations, but can be set using the Initializer method.
func (db *DB) Initialize() {
db.initializer(db)
}

// Initializer changes the initialization function run when the Initialize method is called.
func (db *DB) Initializer(initializer func(*DB)) {
db.initializer = initializer
}

// RunMigrations is an initialization function for a DB which attempts to run migrations
// once a second in a loop until they run successfully.
func RunMigrations(db *DB) {
for {
err := db.Migrate()
if err != nil {
db.Logger.Println(err)
time.Sleep(1 * time.Second)
continue
}
break
}
dbWrap := DB{Client: db}
return dbWrap
}
164 changes: 0 additions & 164 deletions http.go

This file was deleted.

Loading