Optional logger #302
Optional logger #302
Conversation
Signed-off-by: Ana Hobden <[email protected]>
src/raw_node.rs
Outdated
| @@ -216,10 +216,14 @@ pub struct RawNode<T: Storage> { | |||
| prev_hs: HardState, | |||
| } | |||
|
|
|||
| impl<T: Storage> RawNode<T> { | |||
| impl<'a, T: Storage> RawNode<T> { | |||
There was a problem hiding this comment.
Do you want to tie this lifetime to the fn not the impl?
There was a problem hiding this comment.
Oh, yes, you're right! Thanks!
|
I think just showing how to construct a default logger in example is enough. The purpose to require slog logger is to simplify dependencies and interfaces. This PR just defeats the goal. |
|
LGTM. |
|
@BusyJay The case @LucioFranco originally brought up was that they did not wish to use slog in their own program. I think that's a valid case and our current design prevents this. Choice in consensus library should dictate your choice in logger. This allows downstreams like TiKV to opt out of this part of the dependency graph easily, while still allowing users who prefer libraries like tokio-tracing to use it without hacks by falling back to the standard log adapters. I'd prefer to see this PR be an overall migration to tokio-tracing, but that's a bigger task in general. |
src/raft.rs
Outdated
| let logger = logger.new(o!("raft_id" => c.id)); | ||
|
|
||
| let logger = logger.into(); | ||
| #[cfg(feature = "default-logger")] |
There was a problem hiding this comment.
How about providing a new function with_default_logger instead?
There was a problem hiding this comment.
Yes! I think having new(config, store).with_logger(x) or new(config, store).with_default_logger() is good :)
There was a problem hiding this comment.
Oh, I mean with_default_logger works like with_capacity as a constructor.
There was a problem hiding this comment.
I don't think this is a better solution but since it doesn't really matter (We'll move off slog later anyways) letr's do it your way and get this merged.
|
Absolutely agree Jay. :) |
|
@BusyJay I was tracking support right now, and it seems like log -> slog structured conversion may do mallocs right now according to rust-lang/log#328 (comment). It may be worth us using this interm solution for now. |
|
@Hoverbear I think you're right. Sounds like we are very close to getting it in but its not ready yet. I would say this solution seems fine by me. I will also say for now I've written a simple |
|
OK, great, I'll update this PR with #302 (comment) and we can merge this :) |
Signed-off-by: Ana Hobden <[email protected]>
Hoverbear
force-pushed
the
optional-logger
branch
from
26a229a
to
cc35e2a
Compare
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
Signed-off-by: Ana Hobden <[email protected]>
This builds off #288 and fixes #301 .
Could you have a look @LucioFranco ?
Exposes a (default) feature called
default-loggerwhich is our normaltest_loggerjust exposed publicly, it's used whenNoneis passed as a logger toRawNodeorRaft.I am not sure about the exact details about what
default_loggershould look like, but it seems to work fine if I use it as the logger infive_mem_node.