session: allow log configuration using Logger interface. #278
Conversation
Kulezi
force-pushed
the
pp/logger
branch
3 times, most recently
from
bcb363a to
dffd72c
Compare
transport/logger.go
Outdated
| @@ -0,0 +1,69 @@ | |||
| package transport | |||
There was a problem hiding this comment.
how does logger relates to transport? It feels like it belongs to root package
There was a problem hiding this comment.
Almost all logging takes place in the transport package, and putting the entire logger inside of scylla package creates an import cycle, transport package should not have dependencies in scylla package.
I suppose Logger interface could be declared identically in both transport and scylla packages instead of aliasing it in scylla package so it shows up as the whole declaration instead of an alias in docs.
It also could have its own package but that seems excessive to me.
There was a problem hiding this comment.
Almost all logging takes place in the transport package,
Fact that given package is the biggest client of some package doesn't mean it should be defined there. The point of having separate packages is to split functionality in smaller portions so that they can be consumed without unnecessary bloat.
putting the entire logger inside of scylla package creates an import cycle, transport package should not have dependencies in scylla package.
Then it feels like it deserves a separate package.
I suppose Logger interface could be declared identically in both transport and scylla packages instead of aliasing it in scylla package so it shows up as the whole declaration instead of an alias in docs.
Having "same" declaration in multiple places makes it harder to introduce changes to shared part.
It also could have its own package but that seems excessive to me.
Again, transport has nothing in common with logging. It's just wrong to define it here and then type alias on other places.
Kulezi
force-pushed
the
pp/logger
branch
4 times, most recently
from
e8791b4 to
ec3977c
Compare
transport/conn.go
Outdated
| if err := ctx.Err(); err != nil { | ||
| return nil, fmt.Errorf("aborted opening connection on shard %d: %w", si.Shard, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Does the second commit (transport: conn, abort OpenShardConn faster when context is done) belong to this PR? It looks like it can be submitted separately and reviewed independently.
logger/logger.go
Outdated
| @@ -0,0 +1,69 @@ | |||
| package logger | |||
There was a problem hiding this comment.
| package logger | |
| package log |
"log" is in sync with other logging packages
Kulezi
force-pushed
the
pp/logger
branch
2 times, most recently
from
3675270 to
65e6570
Compare
session.go
Outdated
| func NewDefaultLogger() log.Logger { | ||
| return log.NewDefaultLogger() | ||
| } | ||
|
|
||
| func NewDebugLogger() log.Logger { | ||
| return log.NewDebugLogger() | ||
| } | ||
|
|
||
| func NewNopLogger() log.Logger { | ||
| return log.NopLogger{} | ||
| } | ||
|
|
There was a problem hiding this comment.
These shouldn't be exposed in root package, users aren't generally interested such details, default session should use default logger. Advanced users who would like to change logging details, can import log package.
The driver now provides a Logger interface allowing to supply a custom logger as part of (Session)ConnConfig. Driver also provides three implementations of Logger by itself: - DefaultLogger, logging only warnings - DebugLogger, logging everything that can be useful for debugging, it is used by default in tests - NopLogger, logging nothing Fixes #271
This PR provides a Logger interface allowing to supply
a custom logger as part of (Session)ConnConfig.
It also provides three implementations of Logger:
it is used by default in tests
Fixes #271