Warm Index local file delete failure fix #19110
Conversation
himkak
requested review from
anasalkouz,
andrross,
ashking94,
Bukhtawar,
CEHENKLE,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
Rishikesh1159,
sachinpkale,
saratvemulapalli,
shwetathareja,
sohami,
VachaShah and
a team
as code owners
|
❌ Gradle check result for e7aedd6: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
❌ Gradle check result for a2a96c9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
… creating the compound file) fails, we handle in the similar way as is done in FSDriectory, by asking the localDirectory to delete the file. The localDirectory if it fails, already has the mechanism to handle this by maintaining the file's info in pendingDeletes. The pending deletes are taken care later by retriggering the deletes. Signed-off-by: Himanshu Kakar <hkakar@microsoft.com>
Signed-off-by: Himanshu Kakar <hkakar@microsoft.com>
himkak
force-pushed
the
himkak/warmIndexLocalFileDeletionFailure
branch
from
a2a96c9 to
026e3cd
Compare
|
❌ Gradle check result for 026e3cd: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
A few general comments:
Another point I am thinking is if these failures are so generic then why they haven't been caught in our UTs and ITs - we might need to check flakiness of these tests and whether they are running on WindowsFS systems or not. |
| fileCache.remove(blockFilePath); | ||
| }catch(Exception e){ | ||
| if (e.getCause() instanceof AccessDeniedException) { | ||
| localDirectory.deleteFile(blockFile); |
There was a problem hiding this comment.
Better to add a debug log as well on this, might prove useful for understanding how frequent the deletions fail
| try { | ||
| fileCache.remove(blockFilePath); | ||
| }catch(Exception e){ | ||
| if (e.getCause() instanceof AccessDeniedException) { |
There was a problem hiding this comment.
Is there any way we can catch a specific type of Exception in catch instead of getting the exception from cause and doing an instanceof check ?
We should avoid instanceof checks if possible.
Description
For the WarmIndex if the file deletion (which is triggered as part of creating the compound file) fails, we handle in the similar way as is done in FSDriectory, by asking the localDirectory to delete the file. The localDirectory if it fails, already has the mechanism to handle this by maintaining the file's info in pendingDeletes. The pending deletes are taken care later by retriggering the deletes.
Related Issues
Resolves #19052
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.