check backup directory exists before listing children #1895
Conversation
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@ajkr updated the pull request - view changes - changes since last import |
| return status; | ||
| Status status = env->FileExists(dir); | ||
| if (status.ok()) { | ||
| status = env->GetChildrenFileAttributes(dir, &files_attrs); |
There was a problem hiding this comment.
What will happen if we call this function and the directory doesn't exist? Although I'm OK with the approach now, if we can avoid to add this extra HDFS namenode call, it will be better. Every HDFS namenode call is expensive and slow.
There was a problem hiding this comment.
The HDFS client logs an error saying the file doesn't exist. Two customers got confused by these extraneous errors in their logs over the 1-2 weeks, so I'm trying to get rid of them.
At least for this patch, it won't add significant number of calls since this function is only used a few places when initializing engine or creating a new backup.
There was a problem hiding this comment.
@siying you mentioned you're ok with it now, did you mean to accept? Or let me know if you have any more thoughts.
|
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
InsertPathnameToSizeBytes() is called on shared/ and shared_checksum/ directories, which only exist for certain configurations. If we try to list a non-existent directory's contents, some Envs will dump an error message. Let's avoid this by checking whether the directory exists before listing its contents.
Test Plan: used
ldb backupwith internal HDFS env, verified ".../shared_checksum/ not found" error is no longer printed