Skip to content
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.

Sign up for GitHub

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

Delete empty shard folders when DB is deleted #54

Merged
merged 2 commits into from
Aug 26, 2022
Merged

Delete empty shard folders when DB is deleted #54

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
15dea42
Delete empty shard folders when DB is deleted
danydrouin Aug 23, 2022
ea69ba2
Delete dir only for delete DB cleanup
danydrouin Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/main/scala/com/cloudant/clouseau/IndexCleanupService.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class IndexCleanupService(ctx: ServiceContext[ConfigurationArgs]) extends Servic
case CleanupPathMsg(path: String) =>
val dir = new File(rootDir, path)
logger.info("Removing %s".format(path))
recursivelyDelete(dir)
recursivelyDelete(dir, true)
case RenamePathMsg(dbName: String) =>
val srcDir = new File(rootDir, dbName)
val sdf = new SimpleDateFormat("yyyyMMdd'.'HHmmss")
Expand Down Expand Up @@ -65,16 +65,19 @@ class IndexCleanupService(ctx: ServiceContext[ConfigurationArgs]) extends Servic
case 'ok =>
'ok
case ('error, 'not_found) =>
recursivelyDelete(fileOrDir)
recursivelyDelete(fileOrDir, false)
fileOrDir.delete
}
}
}

private def recursivelyDelete(fileOrDir: File) {
if (fileOrDir.isDirectory)
private def recursivelyDelete(fileOrDir: File, deleteDir: Boolean) {
if (fileOrDir.isDirectory) {
for (file <- fileOrDir.listFiles)
recursivelyDelete(file)
recursivelyDelete(file, deleteDir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should actually always specify deleteDir = true, regardless of the recursivelyDelete caller-passed value. (At least in this case.)

The key issue arises from deleting the top-level directory (i.e., ${SEARCH_ROOT}/shards/${RANGE}/${DB_NAME_AND_SUFFIX}) if it temporarily becomes empty during a user changing a ddoc (e.g., when clouseau is cleaning up an orphaned index signature concurrently with trying to create a directory for a new index signature). However, we do typically want to clean-up the empty ${DDOC_INDEX_SIG} directories because deletion only takes place if a signature has changed.

Passing deleteDir = false ensures that any user changing their ddocs burns through inodes by leaving the subdirectories associated with the orphaned signatures on disk.

However, this whole thing is subtle, and I'm happy to hear it argued differently. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about this also made me raise #55 because I think this may be a damned if we do/damned if we don't situation: regardless of the value of deleteDir when recursing, I believe it's possible for CleanupDbMsg to actually delete newly created Lucene files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on the usecase.

For our application we create and delete many DBs. This leads to a huge amount of directories left behind and thus the inodes leak.

We hardly ever update our design docs and index definitions. So in our particular case CleanupDbMsg is less of an issue.

At this stage im more interested in having CleanupPathMsg actually cleanup the leftovers. This is why i chose to set the deleteDir = false for CleanupDbMsg scenario, so to not impact it's current behavior (even if it's problematic today). I do understand the need to fix CleanupDbMsg as well which can probably be done via #55 at a future stage.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it depends on the usecase.

Not really, it's partly about correctness and partly about maintaining stable behaviour for large-scale deployments (as with the one I'm considering).

I understand that your current use case is different, but effectively the change introduces a regression for users who do update their index definitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

im failing to understand why you claim this will cause a regression for "updates" to index definitions.

I didn't change the CleanupDbMsg scenario at all.. it behaves as it did before. It will NOT delete the directories.

Only a the CleanupPathMsg is changed to delete the directories which is from my testing only called when a DB is deleted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, sorry. (I lost track of the fact that the subdirectories weren't deleted before.)

I'm okay with this!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theburge Thanks.. glad we are all clear now.

Can you approve and run the workflow. thanks!

if (deleteDir)
fileOrDir.delete
}
if (fileOrDir.isFile)
fileOrDir.delete
}
Expand Down