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
fix(RepositoryUpdater): make sure temp directories are deleted #2010
Changes from 1 commit
60c5ab1
ba11afb
21626cb
881cf90
b5bfab3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
package org.knora.webapi.store.triplestore.upgrade | ||
|
||
import java.nio.file.{Files, Path, Paths} | ||
import java.io.File | ||
import scala.reflect.io.Directory | ||
|
||
import akka.actor.{ActorRef, ActorSystem} | ||
import akka.http.scaladsl.util.FastFuture | ||
|
@@ -72,6 +74,25 @@ class RepositoryUpdater( | |
log = log | ||
) | ||
|
||
private val tempDirNamePrefix: String = "knora" | ||
|
||
/** | ||
* Deletes previously created temp directories if these were not deleted after update. | ||
* It could happen if update crashed before ends. | ||
*/ | ||
def deleteTempDirectories(): Unit = { | ||
val rootDir = new File("/tmp/") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think adding that check doesn't make sense since that part of code is commented out and Hmmm, is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @subotic I've removed unused code in 881cf90. I don't know why it was commented out, but if my implementation will not work, it can be brought back. That means maybe instead of creating random folders, only one would be used. I've tried out And There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please don't merge a PR if the discussion is not finished. The locally defined There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm sorry but thought you are not going to reply and because already accepting this PR there isn't any critical issue here. If you feel so, this changes could be reverted. But this isn't deployed directly to the production, we have TEST and STAGE too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just checked PROD configuration and apparently |
||
val getTempToDelete = rootDir.listFiles.filter(_.getName.startsWith(tempDirNamePrefix)) | ||
|
||
if (getTempToDelete.length != 0) { | ||
getTempToDelete.foreach(dir => { | ||
val dirToDelete = new Directory(dir) | ||
dirToDelete.deleteRecursively() | ||
}) | ||
log.info(s"Deleted temp directories: ${getTempToDelete.map(_.getName()).mkString(", ")}") | ||
} | ||
} | ||
|
||
/** | ||
* Updates the repository, if necessary, to work with the current version of Knora. | ||
* | ||
|
@@ -83,18 +104,20 @@ class RepositoryUpdater( | |
requiredRepositoryVersion = org.knora.webapi.KnoraBaseVersion | ||
|
||
// Is the repository up to date? | ||
repositoryUpToData = foundRepositoryVersion.contains(requiredRepositoryVersion) | ||
repositoryUpToDate: Boolean = foundRepositoryVersion.contains(requiredRepositoryVersion) | ||
|
||
repositoryUpdatedResponse: RepositoryUpdatedResponse <- | ||
if (repositoryUpToData) { | ||
if (repositoryUpToDate) { | ||
// Yes. Nothing more to do. | ||
FastFuture.successful(RepositoryUpdatedResponse(s"Repository is up to date at $requiredRepositoryVersion")) | ||
} else { | ||
// No. Construct the list of updates that it needs. | ||
|
||
log.info( | ||
s"Repository not up-to-date. Found: ${foundRepositoryVersion.getOrElse("None")}, Required: $requiredRepositoryVersion" | ||
) | ||
|
||
deleteTempDirectories() | ||
|
||
val selectedPlugins: Seq[PluginForKnoraBaseVersion] = selectPluginsForNeededUpdates(foundRepositoryVersion) | ||
log.info(s"Updating repository with transformations: ${selectedPlugins.map(_.versionString).mkString(", ")}") | ||
|
||
|
@@ -168,14 +191,15 @@ class RepositoryUpdater( | |
val downloadDir: Path = settings.upgradeDownloadDir match { | ||
case Some(configuredDir) => | ||
// Yes. Use that directory. | ||
// TODO-mpro never used - see application.conf line 341 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this comment be deleted? The content of application.conf can vary from one setup to another, so the reference to a certain line and value in that conf file may not be valid anymore. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You know what, actually you had right. It doesn't make sense., because we never get back to it (at least here). |
||
log.info(s"Repository update using configured download directory $configuredDir") | ||
val dirFile = Paths.get(configuredDir) | ||
Files.createDirectories(dirFile) | ||
dirFile | ||
|
||
case None => | ||
// No. Create a temporary directory. | ||
val dirFile = Files.createTempDirectory("knora") | ||
val dirFile = Files.createTempDirectory(tempDirNamePrefix) | ||
log.info(s"Repository update using download directory $dirFile") | ||
dirFile | ||
} | ||
|
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.
In my opinion there is too much logic in this docstring. I would just describe the method with what it is actually doing, something like: "Deletes directories inside temp directory starting with tempDirNamePrefix."
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.
True, created docstring before moving filename prefix to the
tempDirNamePrefix
.