[core] Spdlog FD sink #50144
[core] Spdlog FD sink #50144
Conversation
Signed-off-by: dentiny <[email protected]>
dentiny
force-pushed
the
hjiang/spdlog-fd-sink
branch
from
739bf5f to
f6cbd36
Compare
| template <typename Mutex> | ||
| class fd_sink final : public base_sink<Mutex> { | ||
| public: | ||
| // [fd] is not owned by [FdSink], which means the file descriptor should be closed by |
There was a problem hiding this comment.
Q: how do we plan to manage lifetime of fd, if the sink does not own it? can we design it to let sink hold lifetime of fd and close it on sink close?
There was a problem hiding this comment.
As I documented, fd not owned by logger, and should be closed by callers.
There was a problem hiding this comment.
For example, the ideal use case for now is for stdout and stderr, which we don't need to manage inside of the sink; if we do need to transfer ownership in the future, we could add a boolean flag to enable/disable.
But I don't think it's a strong need, because general-purpose files could use RotatingFileSink; FdSink should only handle special-purpose file types.
There was a problem hiding this comment.
Actually have the same question: do we have a use case where we want the fd to outlive the sink? Seems it's easier to move the fd to the sink and let the sink be the owner of the fd.
There was a problem hiding this comment.
stdout and stderr doesn't need to be closed
src/ray/util/spdlog_fd_sink.h
Outdated
| #include <windows.h> | ||
| #endif | ||
|
|
||
| namespace spdlog::sinks { |
There was a problem hiding this comment.
I'm not sure it's best practice to put our classes in spdlog namespace. maybe just namespace ray?
There was a problem hiding this comment.
I prefer to place it under spdlog namespace, so we could use it as other sinks.
For example,
spdlog::sinks::rotating_sink file_sink{...};
spdlog::sinks::fd_sink stdout_sink{duped_stdout};
spdlog::sinks::fd_sink stderr_sink{duped_stderr};
There was a problem hiding this comment.
The coding practice, including naming (use snake_case instead of CamelCase for class name) and type alias both follows spdlog style.
src/ray/util/spdlog_fd_sink.h
Outdated
| }; | ||
|
|
||
| using fd_sink_mt = fd_sink<std::mutex>; | ||
| using fd_sink_st = fd_sink<details::null_mutex>; |
There was a problem hiding this comment.
Yes, I'm following the code practice as other sink implementations.
https://github.com/gabime/spdlog/blob/ae1de0dc8cf480f54eaa425c4a9dc4fef29b28ba/include/spdlog/sinks/rotating_file_sink.h#L56-L57
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
|
merge conflict |
Signed-off-by: dentiny <[email protected]>
| template <typename Mutex> | ||
| class fd_sink final : public base_sink<Mutex> { | ||
| public: | ||
| // [fd] is not owned by [FdSink], which means the file descriptor should be closed by |
There was a problem hiding this comment.
Actually have the same question: do we have a use case where we want the fd to outlive the sink? Seems it's easier to move the fd to the sink and let the sink be the owner of the fd.
Signed-off-by: dentiny <[email protected]>
Signed-off-by: dentiny <[email protected]>
| using non_owned_fd_sink_mt = non_owned_fd_sink<std::mutex>; | ||
| using non_owned_fd_sink_st = non_owned_fd_sink<spdlog::details::null_mutex>; |
There was a problem hiding this comment.
what's the difference between these two?
There was a problem hiding this comment.
multithreaded version and single threaded version, it's compatible to spdlog's implementation, example https://github.com/gabime/spdlog/blob/ae1de0dc8cf480f54eaa425c4a9dc4fef29b28ba/include/spdlog/sinks/rotating_file_sink.h#L56-L57
| #if defined(__APPLE__) || defined(__linux__) | ||
| int GetStdoutHandle() { return STDOUT_FILENO; } | ||
| #elif defined(_WIN32) | ||
| HANDLE GetStdoutHandle() { return GetStdHandle(STD_OUTPUT_HANDLE); } | ||
| #endif |
There was a problem hiding this comment.
I think we already define this somewhere else?
There was a problem hiding this comment.
I'm working on a PR right now to have compatible function for file descriptor / handle on windows and unix platform, including: (1) file descriptor; (2) write/flush/read/close syscalls. We could migrate afterwards
Followup PR for #50044
As proposed by @rynewang , wrap file descriptor (stdout/stderr in the context) into a spdlog sink, so we don't need to have extra flush and close function for
StreamRedirectionHandle, and everything should be handled by spdlog logger.