cw20050111-prog commented on issue #2696: URL: https://github.com/apache/brpc/issues/2696#issuecomment-5422641177
Hi @chenBright, I'd like to pick up the log rotation part of this issue, if it's still unclaimed. I checked current master: butil/logging still has no rotation, and the LogStream / LogStreamFactory refactor discussed above hasn't landed either. You mentioned in July 2024 that you had plans to support rotation in butil/logging — before I start, I'd like to make sure I wouldn't be duplicating work you've already done locally. A rough sketch of what I have in mind for a first PR: - Scope: size-based rotation only. Time-based rotation, compression and "current" symlinks would be follow-ups, to keep the first change reviewable. - Flags: something like `--log_rotate_size_mb` (default 0, meaning disabled) and `--log_rotate_max_backups`. Deliberately *not* `--max_log_size`, since glog already defines that flag and it would collide under BRPC_WITH_GLOG. - Hook point: `Log2File()`. It is the single funnel for both the synchronous path (DefaultLogSink) and the asynchronous one (AsyncLogger::DoLog), so rotation lands in the file-writing layer rather than in any sink. That should also keep it orthogonal to a future LogStream/LogStreamFactory refactor. - The file is opened with "a", so the initial size has to come from fstat/ftell rather than being counted from zero after a restart. - Default off, so upgrading changes neither file names nor existing files. - Whole thing guarded by `#if !BRPC_WITH_GLOG`. Two questions before I write any code: 1. What's the current status of your rotation plan — do you already have an implementation, or is this open for someone else to take? 2. Would you rather see the LogStream/LogStreamFactory refactor land first? If the plan is for rotation to eventually live behind that abstraction, I can wait, or shape the patch so it moves cleanly afterwards. One design question I'd also like your opinion on: multi-process. On POSIX `LoggingLock` uses a process-local pthread mutex, so multiple processes writing to the same log file aren't serialized today, and a rename+reopen would leave other processes writing to the old inode. I'd lean towards documenting multi-process shared log files as unsupported for rotation rather than adding flock, but I'd rather follow your preference here. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
