[core] pipe logging in C++ - #49733
Conversation
edf17ac to
7a0f7d4
Compare
1803428 to
f6edaff
Compare
Signed-off-by: dentiny <dentinyhao@gmail.com>
f6edaff to
ff714c0
Compare
| struct PipeStreamToken { | ||
| // Used to write to. | ||
| // | ||
| // TODO(hjiang): I will followup with another PR to make a `FD` class, which is not |
There was a problem hiding this comment.
what does this mean? Even if a fd as int is copied it does not automatically dup. Can you give an example of how do you plan to have the FD class?
See also: MEMFD_TYPE, INT2FD, FD2INT in compat.h
There was a problem hiding this comment.
I think you can just use MEMFD_TYPE and don't need the ifdef here
There was a problem hiding this comment.
Yes, I would like to wrap file descriptor in a class, which
- Disallow copy
- Allow move, which indicates resource ownership transfer
- Close fd at dtor
The biggest motivation is to avoid manual dup from unintentional copy.
There was a problem hiding this comment.
yeah we can do it in another pr
There was a problem hiding this comment.
See also: MEMFD_TYPE, INT2FD, FD2INT in compat.h
I leave a TODO for using MEMFD, because we don't have a separate target for compat.h, introducing which means pulling heavy dependencies.
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
| struct PipeStreamToken { | ||
| // Used to write to. | ||
| // | ||
| // TODO(hjiang): I will followup with another PR to make a `FD` class, which is not |
There was a problem hiding this comment.
yeah we can do it in another pr
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
Signed-off-by: dentiny <dentinyhao@gmail.com>
…ang/pipe-logging Signed-off-by: dentiny <dentinyhao@gmail.com>
6606812 to
86f0481
Compare
rynewang
left a comment
rynewang
left a comment
There was a problem hiding this comment.
I find this design over engineered:
- It provides a on_completion callback which nobody uses other than the unit test.
- It spawns 2 threads: 1 to listen the fd and write to a buffer; 1 to dump the buffer content to spdlog. I think 1 thread can already do the work, only on some curated "log stress tests" where the user write megabytes of data to stdout will a single thread solution be blocked by the backend write-to-disk operation. And in this case, even a simple "stdout > 1.log" will block as well so the blocking should be expected by the user anyway.
That said, this PR is delayed for too long. I choose to merge it as long as a future maintainer can find the message in this PR, because its over-engineering is (mostly) contained within the library itself and it's not likely to change dramatically, so the pain is contained.
| // Open the file with read-only access | ||
| int fd = open(fname.c_str(), O_RDONLY); | ||
| RAY_CHECK_GT(fd, 0); | ||
| BOOST_SCOPE_EXIT(&fd) { close(fd); } |
There was a problem hiding this comment.
We can use absl::Cleanup ?
There was a problem hiding this comment.
Yeah I think they are the same thing.
There was a problem hiding this comment.
then let's prefer the non-macro one
For the callback, I think it's needed in production; |
|
the dtor itself should do the flushing? |
dtor performs flushing, atexit perform flush synchronization, the callback is needed based on the async logging nature |
This PR implements the pipe logging in C++, instead of python.
The main motivation for reuse for other languages in the future.
One thing worth noticing is I use two backgrounds for read from pipe and
dump to spdlog:
- The motivation is to avoid any IO operation on read thread, thus
blocking write thread due to limited pipe buffer size
- The overhead of which is extra copy of logging message; we should
merge into one thread if performance issue reported
Usage in core worker:
```
class CoreWorker {
// Only assigned when rotation is requested; otherwise empty;
// All handles' termination functors are invoked at core workers' destruction.
std::vector<RotationFileHandle> rotation_file_handles_;
};
```
---------
Signed-off-by: dentiny <dentinyhao@gmail.com>
This PR implements the pipe logging in C++, instead of python.
The main motivation for reuse for other languages in the future.
One thing worth noticing is I use two backgrounds for read from pipe and
dump to spdlog:
- The motivation is to avoid any IO operation on read thread, thus
blocking write thread due to limited pipe buffer size
- The overhead of which is extra copy of logging message; we should
merge into one thread if performance issue reported
Usage in core worker:
```
class CoreWorker {
// Only assigned when rotation is requested; otherwise empty;
// All handles' termination functors are invoked at core workers' destruction.
std::vector<RotationFileHandle> rotation_file_handles_;
};
```
---------
Signed-off-by: dentiny <dentinyhao@gmail.com>
5 participants
Address @jjyao 's comment: #49548 (comment)
This PR implements the pipe logging in C++, instead of python.
The main motivation for reuse for other languages in the future.
One thing worth noticing is I use two backgrounds for read from pipe and dump to spdlog:
Usage in core worker: