hubcio commented on code in PR #2452:
URL: https://github.com/apache/iggy/pull/2452#discussion_r2652324017
##########
core/configs/server.toml:
##########
@@ -366,6 +366,13 @@ file_enabled = true
# Maximum size of the log files before rotation.
max_size = "512 MB"
Review Comment:
this is misleading if you look at lines 369 to 371 - please rename to
`max_file_size` and update description (assuming i'm right)
##########
core/server/src/configs/system.rs:
##########
@@ -88,12 +88,26 @@ pub struct LoggingConfig {
pub level: String,
pub file_enabled: bool,
pub max_size: IggyByteSize,
+ #[serde(default = "default_max_total_log_size")]
+ pub max_total_size: IggyByteSize,
+ #[serde(default = "default_log_rotation_check_interval")]
+ pub rotation_check_interval: u64,
Review Comment:
change to `IggyByteSize`
##########
core/server/src/configs/system.rs:
##########
@@ -88,12 +88,26 @@ pub struct LoggingConfig {
pub level: String,
pub file_enabled: bool,
pub max_size: IggyByteSize,
+ #[serde(default = "default_max_total_log_size")]
+ pub max_total_size: IggyByteSize,
+ #[serde(default = "default_log_rotation_check_interval")]
+ pub rotation_check_interval: u64,
#[serde_as(as = "DisplayFromStr")]
pub retention: IggyDuration,
#[serde_as(as = "DisplayFromStr")]
pub sysinfo_print_interval: IggyDuration,
}
+fn default_max_total_log_size() -> IggyByteSize {
+ IggyByteSize::from(4_000_000_000)
+ // 4 GB by default
+}
+
+fn default_log_rotation_check_interval() -> u64 {
+ 3600
+ // 1 hour by default
+}
Review Comment:
this is now how we do the defaults, we use values from actual config in repo
and you already did it, so you can remove these, along with line 91 and 93.
##########
core/server/src/configs/defaults.rs:
##########
@@ -403,6 +403,8 @@ impl Default for LoggingConfig {
level: SERVER_CONFIG.system.logging.level.parse().unwrap(),
file_enabled: SERVER_CONFIG.system.logging.file_enabled,
max_size: SERVER_CONFIG.system.logging.max_size.parse().unwrap(),
+ max_total_size:
SERVER_CONFIG.system.logging.max_total_size.parse().unwrap(),
+ rotation_check_interval:
SERVER_CONFIG.system.logging.rotation_check_interval as u64,
Review Comment:
please use IggyDuration, set default value to "1 h" and in `impl Validate
for LoggingConfig` check if its higher than `1 s`
##########
core/server/src/log/logger.rs:
##########
@@ -397,26 +444,199 @@ impl Logging {
Format::default().with_thread_names(true)
}
- fn _install_log_rotation_handler(&self) {
- todo!("Implement log rotation handler based on size and retention
time");
- }
-
fn print_build_info() {
if option_env!("IGGY_CI_BUILD") == Some("true") {
let hash = option_env!("VERGEN_GIT_SHA").unwrap_or("unknown");
let built_at =
option_env!("VERGEN_BUILD_TIMESTAMP").unwrap_or("unknown");
let rust_version =
option_env!("VERGEN_RUSTC_SEMVER").unwrap_or("unknown");
let target =
option_env!("VERGEN_CARGO_TARGET_TRIPLE").unwrap_or("unknown");
info!(
- "Version: {VERSION}, hash: {}, built at: {} using rust
version: {} for target: {}",
- hash, built_at, rust_version, target
+ "Version: {VERSION}, hash: {hash}, built at: {built_at} using
rust version: {rust_version} for target: {target}"
);
} else {
info!(
"It seems that you are a developer. Environment variable
IGGY_CI_BUILD is not set to 'true', skipping build info print."
)
}
}
+
+ fn calculate_max_files(max_total_size_bytes: u64, max_file_size_bytes:
u64) -> usize {
+ if max_file_size_bytes == 0 {
+ return 10;
+ }
+
+ let max_files = max_total_size_bytes / max_file_size_bytes;
+ max_files.clamp(1, 1000) as usize
+ }
+
+ fn _install_log_rotation_handler(&self, config: &LoggingConfig, logs_path:
Option<&PathBuf>) {
+ if let Some(logs_path) = logs_path {
+ let path = logs_path.to_path_buf();
+ let max_size = config.max_size.as_bytes_u64();
+ let retention = config.retention.get_duration();
+ let check_interval = config.rotation_check_interval;
+ let should_stop = Arc::clone(&self.rotation_should_stop);
+
+ std::thread::spawn(move || {
Review Comment:
please name the thread, like
```rust
let handle = std::thread::Builder::new()
.name("log-rotation".to_string())
.spawn(move || { ... })
.expect("Failed to spawn log rotation thread");
```
also, correct me if im wrong, but proper shutdown is still missing. how can
we know if this thread has shutdown, if join is not called: this can lead to
partial cleanup of log files.
--
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]
