Conversation
….log (5M each) with settings proxy.config.log.auto_delete_files_max_space_mb STRING diags.log=7,traffic.out=8,squid.log=10 proxy.config.log.max_space_mb_for_logs INT 25. Verified that 1) it does delete files and 2) deletion happens in order and 3) deletion works not just for the first time.
proxy/logging/LogConfig.cc
Outdated
| rolling_enabled = Log::NO_ROLLING; | ||
| } | ||
|
|
||
| char *limits = REC_ConfigReadString("proxy.config.log.auto_delete_files_space_limits_mb"); |
There was a problem hiding this comment.
Better to use ats_scoped_str in cases like this.
proxy/logging/LogConfig.cc
Outdated
| // Build new LogDeletingInfo based on the [type]=[limit] pair | ||
| deleting_info.insert(new LogDeletingInfo(key, static_cast<int64_t>(ts::svtoi(value)) * LOG_MEGABYTE)); | ||
| } else { | ||
| Warning("invalid key-value pair '%s' for '%s', skipping this pair", value.data(), |
There was a problem hiding this comment.
Have you tested this? Can you depend on value.data() to be null terminated?
proxy/logging/LogConfig.cc
Outdated
| // then add this entry to the candidate list | ||
| // then check if the candidate belongs to any given log types with allowable space | ||
| // | ||
| int len = strchr(strchr(entry->d_name, '.') + 1, '.') - entry->d_name; |
There was a problem hiding this comment.
Why not use TextView here? This assumes the presence of '.' twice and will crash if that's not the case. Maybe
type_name = TextView(entry->d_name, strlen(entry->d_name));
type.name.remove_suffix(TextView(type_name).take_prefix_at('.').take_prefix_at('.')).size());
Alternatively, if it's really the suffix you want to remove, e.g. the name is "A.B.C" and you want "A.B", then
TextView type_name(entry->d_name, strlen(entry->d_name));
type_name.take_suffix_at('.');
proxy/logging/LogConfig.cc
Outdated
|
|
||
| iter->total_size += candidates[candidate_count].size; | ||
| candidate_count++; | ||
| total_candidate_count++; |
proxy/logging/LogConfig.h
Outdated
| -------------------------------------------------------------------------*/ | ||
| struct LogDeleteCandidate { | ||
| time_t mtime; | ||
| char *name; |
There was a problem hiding this comment.
Rather than the explicit cleanup, make this a std::string.
proxy/logging/LogConfig.h
Outdated
| int candidate_count{0}; | ||
| int victim{0}; | ||
| int64_t total_size{0LL}; | ||
| LogDeleteCandidate candidates[MAX_CANDIDATES]; |
There was a problem hiding this comment.
What happens if this is a std::vector? Is there a problem with iterator stability?
proxy/logging/LogConfig.cc
Outdated
| } | ||
|
|
||
| // | ||
| for (auto iter = deleting_info.begin(); iter != deleting_info.end(); iter++) { |
There was a problem hiding this comment.
If you're using IntrusiveHashMap, IntrusiveHashMap::apply is a better choice. Also, should you be deleting the entries in the table?
| candidate_count++; | ||
| ts::TextView type_name(entry->d_name, strlen(entry->d_name)); | ||
| auto suffix = type_name; | ||
| type_name.remove_suffix(suffix.remove_prefix(suffix.find('.') + 1).remove_prefix(suffix.find('.')).size()); |
There was a problem hiding this comment.
This seems rather convoluted. Why not
suffix.take_prefix_at('.');
suffix.take_prefix_at('.');
type_name.remove_suffix(suffix.size());
or if vertical space is short
type_name.remove_suffix((suffix.take_prefix_at('.'), suffix.take_prefix_at('.'), suffix.size()));
|
Seems close enough. |
2 participants
Small logs that rotate more frequently are wiped out and the loss of diagnostic logs make it harder to debug. This pull request brings a new deletion mechanism with a config option of max space allowed for all types of log files. (proxy.config.log.auto_delete_files_space_limits_mb)
The deletion will happen on the oldest file from the log type with biggest ratio=(total size used by the type)/(max space allowed for the type). Log types that are not configured with the max space allowed will not be considered for deletion. (If the config option is empty, then traffic server won't delete any files.)
Test case:
Config:
proxy.config.log.max_space_mb_for_logs INT 25
proxy.config.log.auto_delete_files_space_limits_mb STRING diags.log=7,traffic.out=8,squid.log=10
Files:
10 traffic.out (1M each), 10 diags.log (1M each), and 10 squid.log (5M each)
Verified that 1) it does delete files and 2) deletion happens in order and 3) deletion works not just for the first time.