Issue 18: Housekeeping shouldn't fail when attempting to cleanup a path that is already deleted #19
Conversation
There was a problem hiding this comment.
This looks mostly good, have you tested it with "for real" by manually deleting some paths that are in the DB and running the tool? Should be fairly straightforward to reproduce before/after behaviour if you've managed to get the "tool" into this project (I can't remember where we are with that?)
| @@ -47,39 +48,65 @@ public FileSystemHousekeepingService(LegacyReplicaPathRepository legacyReplicaPa | |||
| LOG.warn("{}.fixIncompleteRecord(LegacyReplicaPath) should be removed in future.", getClass()); | |||
| } | |||
|
|
|||
| private FileSystem fileSystemForPath(LegacyReplicaPath cleanUpPath) { | |||
| LOG.info("Attempting to delete path '{}' from file system", cleanUpPath); | |||
There was a problem hiding this comment.
This isn't doing a delete? Update comment to clarify that this is trying to determine the FS for a path, or just remove it. If you do decide to keep it it's probably worth putting on debug rather than info.
There was a problem hiding this comment.
Moved it to more relevant place, aftermath of copy and paste :)
| Path path = new Path(cleanUpPath.getPath()); | ||
| FileSystem fs; | ||
| try { | ||
| fs = path.getFileSystem(conf); |
There was a problem hiding this comment.
You could just return this and then you don't even need the "fs" variable. The scope expanding out on line 54 definitely isn't needed.
| @@ -47,39 +48,65 @@ public FileSystemHousekeepingService(LegacyReplicaPathRepository legacyReplicaPa | |||
| LOG.warn("{}.fixIncompleteRecord(LegacyReplicaPath) should be removed in future.", getClass()); | |||
| } | |||
|
|
|||
| private FileSystem fileSystemForPath(LegacyReplicaPath cleanUpPath) { | |||
| LOG.info("Attempting to delete path '{}' from file system", cleanUpPath); | |||
| Path path = new Path(cleanUpPath.getPath()); | |||
There was a problem hiding this comment.
This method could take the Path type directly so you don't have to create it twice (once here and once in the calling method currently)
| } | ||
| } | ||
| } catch (Exception e) { | ||
| LOG.warn("Path '{}' was not deleted from the housekeeping database. {}", cleanUpPath, e.getMessage()); |
There was a problem hiding this comment.
This does prevent housekeeping from failing but it won't delete the path from the repo, so we'll get this warning on every run of housekeeping or am I missing something?
There was a problem hiding this comment.
This block is only reached if legacyReplicaPathRepository.delete(cleanUpPath); fails, signifying a failure to delete the specified path from the Housekeeping database
There was a problem hiding this comment.
if oneOfMySiblingsWillTakeCareOfMyAncestors(path, rootPath, fs) || thereIsNothingMoreToDelete(fs, rootPath) throws an exception which I thought was why we got the error in the first place, we'll hit the catch
There was a problem hiding this comment.
Ah right, I was looking at wrong try block.. I'm hesitant to delete data from the Housekeeping database here as it may fail for another unforeseen reason that isn't the path being deleted already, do you think we should just do the deletion anyway?
There was a problem hiding this comment.
I guess we need to know exactly at what point we got the error and check what path we were trying to delete is that some parent path or something else? What would happen if we warn and run again, failure at same place?
There was a problem hiding this comment.
This becomes complex cause of eventual consistency and the likes, I would rather leave it like this, they will get a small warning in their logs when they run their application but I would rather that over leaving behind data that I may be paying for
There was a problem hiding this comment.
ok let's go for this, if the warnings become an issue we can revisit.
Fixes #18