Reduce blob db noisy logging #2587
Conversation
Summary: Remove some of the per-key logging by blob db to reduce noise. Test Plan: Test with db_bench.
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
utilities/blob_db/blob_db_impl.cc
Outdated
| packet.dsn_, packet.file_number_, packet.blob_offset_, | ||
| packet.blob_size_, succ); | ||
| FindFileAndEvictABlob(packet.file_number_, packet.key_size_, | ||
| packet.blob_offset_, packet.blob_size_); |
There was a problem hiding this comment.
Might be good to add a comment here in the code on why you think it is ok to discard the return value and not handle the failure case.
There was a problem hiding this comment.
The only unsuccessful case of FindFileAndEvictABlob is when corresponding file is deleted, which shouldn't consider an error case? I'm going to remove return value of FindFileAndEvictABlob instead.
There was a problem hiding this comment.
Let me keep the return value but logging the total number of failures at the end.
There was a problem hiding this comment.
I looked at the code in FindFileAndEvictABlob just now to see in what cases it returns false, and as you said, it seems to be only in the case where the file is already deleted. So, the code as is, is fine with me.
utilities/blob_db/blob_db_impl.cc
Outdated
| if (s1.IsBusy()) { | ||
| ROCKS_LOG_INFO(db_options_.info_log, | ||
| "Optimistic transaction failed delete: %s bn: %" PRIu32, | ||
| bfptr->PathName().c_str(), gcstats->blob_count); |
There was a problem hiding this comment.
"transaction failed delete" ... this seems like an important log message, isn't it?
Since you have the best knowledge of the code, i'll leave the final decision to you.
All the messages can be removed in successful cases, but its the non-success messages which are always important to be logged, in my view.
There was a problem hiding this comment.
The transaction will fail only when a newer version of the key is being insert, in which case we don't need to delete the key from LSM. So this is not an error case. Logging the info per key is just too noisy. I'm going to add a counter and report it through GCStats.
|
@yiwu-arbug updated the pull request - view changes - changes since last import |
|
@yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Remove some of the per-key logging by blob db to reduce noise. Closes #2587 Differential Revision: D5429115 Pulled By: yiwu-arbug fbshipit-source-id: b89328282fb8b3c64923ce48738c16017ce7feaf
Summary:
Remove some of the per-key logging by blob db to reduce noise.
Test Plan:
Test with db_bench.