-
Notifications
You must be signed in to change notification settings - Fork 60
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
PSMDB-810 improve logging #695
base: v4.2
Are you sure you want to change the base?
Conversation
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.
Hi Igor. As you suggested in our slack chat, I think the special logging should be a log level 0.
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.
Thanks for changing the log level to 0 Igor.
I have some edit suggestions again though sorry.
log() << "Hot backup copying {} bytes from '{}' to '{}'"_format( | ||
fsize, srcFile.string(), destFile.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.
log() << "Hot backup copying {} bytes from '{}' to '{}'"_format( | |
fsize, srcFile.string(), destFile.string()); | |
log() << "Beginning copy of {}/{} files in backup snapshot: {}, {} bytes"_format( | |
fcCtr++, filesList.size(), srcFile.filename(), fsize); |
(The fcCtr variable needs to be defined just a bit above of course.)
As for the destFile I think the destination directory only needs to be shown once as the backup process begins. (The destination incidentally also appears in the argument values of the "command: createBackup" log message when it finishes.)
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 wish we could print the user-recognizable collection or index namespace string, when a file is a user-data table, but I don't find a map of "idents" -> namespaces. Only private member "NSToIdentMap _idents;" in the DurableCatalogImpl class, so we'd also have to change that class to access.
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.
pull request updated
} else { | ||
log() << "Source file size is: {} bytes"_format(fs::file_size(srcFile)); | ||
} | ||
|
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.
} else { | |
log() << "Source file size is: {} bytes"_format(fs::file_size(srcFile)); | |
} | |
} | |
Truncating the byte size log line from here because the preceding edit suggestion would include it there.
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.
My idea was to show current size of the file. It may be different from the size reported in another message (that size is captured when file list is generated)
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.
Thanks for catching the fcCtr increment bug.
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.
Regarding showing the size of the file it is true, it might become notably bigger during the backup. And that is so interesting to DBAs worried about file size growth that I first drafted an edit where it would warn() (not log) if the file size had grown (> +10% && > +100MB).
I don't think it's helpful for users to just be informed what the size is if it is not the same size as what is copied. I think they will see it as an error and report it.
But then I discarded that draft because the $backupCursor work will probably require us doing this all again, didn't seem worthwhile now.
} else { | ||
log() << "Source file size is: {} bytes"_format(fs::file_size(srcFile)); | ||
} | ||
|
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.
Thanks for catching the fcCtr increment bug.
} else { | ||
log() << "Source file size is: {} bytes"_format(fs::file_size(srcFile)); | ||
} | ||
|
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.
Regarding showing the size of the file it is true, it might become notably bigger during the backup. And that is so interesting to DBAs worried about file size growth that I first drafted an edit where it would warn() (not log) if the file size had grown (> +10% && > +100MB).
I don't think it's helpful for users to just be informed what the size is if it is not the same size as what is copied. I think they will see it as an error and report it.
But then I discarded that draft because the $backupCursor work will probably require us doing this all again, didn't seem worthwhile now.
No description provided.