-
Notifications
You must be signed in to change notification settings - Fork 70
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add log Rotation #62
base: master
Are you sure you want to change the base?
Add log Rotation #62
Conversation
Codecov Report
@@ Coverage Diff @@
## master #62 +/- ##
==========================================
- Coverage 88.40% 88.20% -0.21%
==========================================
Files 33 33
Lines 7394 7543 +149
==========================================
+ Hits 6537 6653 +116
- Misses 857 890 +33
Continue to review full report at Codecov.
|
Signed-off-by: Fullstop000 <[email protected]>
src/logger.rs
Outdated
storage: &S, | ||
db_path: &str, | ||
storage: S, | ||
db_path: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db_path
could be a &'static str
I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try
src/logger.rs
Outdated
@@ -120,13 +130,44 @@ fn log_to_slog_level(level: log::Level) -> Level { | |||
|
|||
struct FileBasedDrain<F: File> { | |||
inner: Mutex<F>, | |||
rotators: Vec<Box<dyn Rotator>>, | |||
dp_path: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this db_path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YES,you ar right
src/logger.rs
Outdated
FileBasedDrain { | ||
dp_path: path.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/logger.rs
Outdated
if rotator.is_enabled() { | ||
self.rotators.push(Box::new(rotator)); | ||
} | ||
for rotator in (&self).rotators.iter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use self.rotators.as_ref().iter()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NO, THIS CANNOT COMPILER
src/logger.rs
Outdated
let new_file = (self.new_file)(self.dp_path.clone()).unwrap(); | ||
|
||
let mut old_file = self.inner.lock().unwrap(); | ||
std::mem::replace(&mut *old_file, new_file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be
*old_file = new_file
src/logger.rs
Outdated
); | ||
for rotator in self.rotators.iter() { | ||
if rotator.should_rotate() { | ||
self.flush().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unwrap()
seems not suitable here. At least we can throw the error I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what kind of error should throw,Error:: Corruption
,Error::IO
or Error::Customized
use std::sync::Mutex; | ||
|
||
/// A `slog` based logger which can be used with `log` crate | ||
/// | ||
/// See `slog` at https://github.com/slog-rs/slog | ||
/// See `log` at https://github.com/rust-lang/log | ||
/// | ||
|
||
pub fn create_file<S: Storage>(storage: &S, db_path: &str, timestamp: i64) -> Result<S::F> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems pub
is not required. If using pub
, pls add rustdoc for it :)
/// Return if the file need to be rotated. | ||
fn should_rotate(&self) -> bool; | ||
|
||
fn on_write(&self, buf: &[u8]) -> Result<()>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls add rustdoc
src/logger.rs
Outdated
|
||
struct RotatedFileBySize { | ||
rotation_size: u64, | ||
file_size: Mutex<u64>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use AtomicU64
instead ?
src/logger.rs
Outdated
} | ||
assert_eq!( | ||
true, | ||
file_exists("norotate/000000.log", FileStorage::default()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use FileStorage::default()
here might not be suitable because the sematic of Default()
could change
src/logger.rs
Outdated
use std::thread; | ||
use std::time::Duration; | ||
|
||
fn file_exists(file: impl AsRef<Path>, storage: impl Storage) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this func is not required?
Initial Added and implemented
Rotator
trait #61