Skip to content
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 option to keep large enough info logs in crash loop #5423

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yiwu-arbug
Copy link
Contributor

Summary:
In case application fall in crash loop, RocksDB generates a new info log file on each restart, and purge useful old logs even when keep_log_file_num is set. In these cases the new log files are usually small and useless for debugging. In such cases we can instead keep large enough info log files. Adding two new options keep_large_log_file_num and large_info_log_size for the purpose.

Test Plan:
Run db_bench and kill manually, and observe large info logs are kept.

Signed-off-by: Yi Wu <yiwu@pingcap.com>
@yiwu-arbug yiwu-arbug mentioned this pull request Jun 6, 2019
@yiwu-arbug yiwu-arbug changed the title Add option to keep large enough info logs Add option to keep large enough info logs in crash loop Jun 7, 2019
@maysamyabandeh
Copy link
Contributor

CC @siying to chime in.
My 2 cents is that if we are going to add another option, it could be max_total_info_log_size to keep the old good files if the total size (the old good one and the many small useless ones) does not exceed the threshold. But more importantly we should get @siying's green light on adding another config option.

@siying
Copy link
Contributor

siying commented Jun 13, 2019

My opinion is that, the feature itself is too specific. Even though crash loop is common among RocksDB users, it is still specific to how users use RocksDB. It's the application, not the library, owns the restarting a crashing service. The idea that smaller log files mean a log for crashing and larger ones didn't crash is not robust. Can we instead empower applications to handle everything by themselves? If it is helpful, we can add a function in RocksDB to provide a list of info log files, and before starting a DB, applications can choose to delete smaller files if they like.

Also keep in mind that Logger is an open interface and you can plug in your own and do whatever you want there.

@yiwu-arbug
Copy link
Contributor Author

Thanks for comment, and totally agree application should implement Logger to handle the use of logs. One thing is that right now both DBImpl::PurgeObsoleteFiles and, e.g., AutoRollLogger handle log deletion. Should DBImpl delegate log file management to Logger? Thanks.

@siying
Copy link
Contributor

siying commented Jun 13, 2019

@yiwu-arbug I think it does make sense, though the interface is a little bit hard to determine. For the crash loop case, you don't need this, right?

@yiwu-arbug
Copy link
Contributor Author

@siying I need to double check. I'm not sure if PurgeObsoleteFiles will purge old files on crash loop, too.

@siying
Copy link
Contributor

siying commented Jun 13, 2019

@yiwu-arbug it will, but here is a question: do you crash before this function starts, or after it finishes?

@siying
Copy link
Contributor

siying commented Jun 13, 2019

@siying by "you don't need this", I mean, before opening the DB, you can check log files in the directory and delete them accordingly.

@siying
Copy link
Contributor

siying commented Sep 6, 2019

@yiwu-arbug do we still plan to make progress on this? Or should we close?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants