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

Jump to bottom

refactor: app actor cleanup #2230

Merged
merged 2 commits into from Oct 3, 2022
Merged

refactor: app actor cleanup #2230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
020c3be
refactor: app actor cleanup
subotic Oct 2, 2022
8811fc7
refactor: app actor cleanup
subotic Oct 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions webapi/src/main/scala/org/knora/webapi/core/ActorSystem.scala
View file
Open in desktop
Expand Up @@ -32,21 +32,21 @@ object ActorSystem {
defaultExecutionContext = Some(executionContext)
)
)
.tap(_ => ZIO.logInfo(">>> Acquire Actor System <<<"))
.zipLeft(ZIO.logInfo(">>> Acquire Actor System <<<"))
.orDie

private def release(system: akka.actor.ActorSystem): URIO[Any, actor.Terminated] =
ZIO
.fromFuture(_ => system.terminate())
.tap(_ => ZIO.logInfo(">>> Release Actor System <<<"))
.zipLeft(ZIO.logInfo(">>> Release Actor System <<<"))
.orDie

val layer: ZLayer[AppConfig, Nothing, ActorSystem] =
ZLayer.scoped {
for {
config <- ZIO.service[AppConfig]
context <- ZIO.executor.map(_.asExecutionContext)
actorSystem <- ZIO.acquireRelease(acquire(context))(release _)
actorSystem <- ZIO.acquireRelease(acquire(context))(release)
} yield new ActorSystem {
override val system: akka.actor.ActorSystem = actorSystem
override val cacheServiceSettings: CacheServiceSettings = new CacheServiceSettings(config)
Expand Down
47 changes: 19 additions & 28 deletions webapi/src/main/scala/org/knora/webapi/core/AppServer.scala
View file
Open in desktop
Expand Up @@ -58,8 +58,7 @@ final case class AppServer(
private def upgradeRepository(requiresRepository: Boolean): ZIO[Any, Nothing, Unit] =
for {
_ <- state.set(AppState.UpdatingRepository)
_ <- if (requiresRepository) ru.maybeUpgradeRepository.flatMap(response => ZIO.logInfo(response.message))
else ZIO.unit
_ <- ru.maybeUpgradeRepository.flatMap(response => ZIO.logInfo(response.message)).when(requiresRepository)
_ <- state.set(AppState.RepositoryUpToDate)
} yield ()

Expand All @@ -84,8 +83,7 @@ final case class AppServer(
private def populateOntologyCaches(requiresRepository: Boolean): ZIO[Any, Nothing, Unit] =
for {
_ <- state.set(AppState.LoadingOntologies)
_ <- if (requiresRepository) ar.populateOntologyCaches
else ZIO.unit
_ <- ar.populateOntologyCaches.when(requiresRepository)
_ <- state.set(AppState.OntologiesReady)
} yield ()

Expand All @@ -97,19 +95,15 @@ final case class AppServer(
private def checkIIIFService(requiresIIIFService: Boolean): ZIO[Any, Nothing, Unit] =
for {
_ <- state.set(AppState.WaitingForIIIFService)
_ <- if (requiresIIIFService)
iiifs
.getStatus()
.flatMap(status =>
status match {
case IIIFServiceStatusOK =>
ZIO.logInfo("IIIF service running")
case IIIFServiceStatusNOK =>
ZIO.logError("IIIF service not running") *> ZIO.die(new Exception("IIIF service not running"))
}
)
else
ZIO.unit
_ <- iiifs
.getStatus()
.flatMap {
case IIIFServiceStatusOK =>
ZIO.logInfo("IIIF service running")
case IIIFServiceStatusNOK =>
ZIO.logError("IIIF service not running") *> ZIO.die(new Exception("IIIF service not running"))
}
.when(requiresIIIFService)
_ <- state.set(AppState.IIIFServiceReady)
Comment on lines 96 to 107
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm just wondering if it's actually reasonable to set the AppState to IIIF stuff, if we don't require the IIIF service. So maybe this method could be rewritten to:

private def checkIIIFService(requiresIIIFService: Boolean): ZIO[Any, Nothing, Unit] =
  if (requiresIIIFService) {
    for {
      _      <- state.set(AppState.WaitingForIIIFService)
      status <- iiifs.getStatus()
      _      <- status match {
        case CacheServiceStatusNOK =>
          ZIO.logError("Cache service not running.") *> 
            ZIO.die(new Exception("Cache service not running."))
        case CacheServiceStatusOK =>
          ZIO.unit
      }
      _      <- state.set(AppState.IIIFServiceReady)
    } yield ()
  } else ZIO.unit

optionally, you could also use ZIO.when to test the bool condition. And even ZIO.whenCase to check the status

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, good point. With ZIO I'm actually not sure if we need these states in such a detail at all, or if something waiting for startup is not good enough. Now that we have logging that works, we should quickly figure out where the startup is stuck. So I will merge it as is, and we can create a follow-up issue after we discuss it.

} yield ()

Expand All @@ -119,15 +113,12 @@ final case class AppServer(
private val checkCacheService: ZIO[Any, Nothing, Unit] =
for {
_ <- state.set(AppState.WaitingForCacheService)
_ <- cs.getStatus
.flatMap(status =>
status match {
case CacheServiceStatusNOK =>
ZIO.logError("Cache service not running.") *> ZIO.die(new Exception("Cache service not running."))
case CacheServiceStatusOK =>
ZIO.unit
}
)
_ <- cs.getStatus.flatMap {
case CacheServiceStatusNOK =>
ZIO.logError("Cache service not running.") *> ZIO.die(new Exception("Cache service not running."))
case CacheServiceStatusOK =>
ZIO.unit
}
_ <- state.set(AppState.CacheServiceReady)
} yield ()

Expand All @@ -141,7 +132,7 @@ final case class AppServer(
def start(
requiresAdditionalRepositoryChecks: Boolean,
requiresIIIFService: Boolean
) =
): ZIO[Any, Nothing, Unit] =
for {
_ <- ZIO.logInfo("=> Startup checks initiated")
_ <- checkTriplestoreService
Expand All @@ -152,7 +143,7 @@ final case class AppServer(
_ <- checkCacheService
_ <- ZIO.logInfo("=> Startup checks finished")
_ <- ZIO.logInfo(s"DSP-API Server started: ${appConfig.knoraApi.internalKnoraApiBaseUrl}")
_ <- if (appConfig.allowReloadOverHttp) ZIO.logWarning("Resetting DB over HTTP is turned ON") else ZIO.unit
_ <- ZIO.logWarning("Resetting DB over HTTP is turned ON").when(appConfig.allowReloadOverHttp)
_ <- state.set(AppState.Running)
} yield ()
}
Expand Down
4 changes: 2 additions & 2 deletions webapi/src/main/scala/org/knora/webapi/core/HttpServer.scala
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ object HttpServer {
.fromFuture(_ =>
Http().newServerAt(config.knoraApi.internalHost, config.knoraApi.internalPort).bind(apiRoutes.routes)
)
.tap(_ => ZIO.logInfo(">>> Acquire HTTP Server <<<"))
.zipLeft(ZIO.logInfo(">>> Acquire HTTP Server <<<"))
.orDie
} { serverBinding =>
ZIO
Expand All @@ -43,7 +43,7 @@ object HttpServer {
new scala.concurrent.duration.FiniteDuration(1, scala.concurrent.duration.MILLISECONDS)
)
)
.tap(_ => ZIO.logInfo(">>> Release HTTP Server <<<"))
.zipLeft(ZIO.logInfo(">>> Release HTTP Server <<<"))
.orDie
}
}
Expand Down
3 changes: 1 addition & 2 deletions webapi/src/main/scala/org/knora/webapi/core/State.scala
View file
Open in desktop
Expand Up @@ -13,8 +13,7 @@ import org.knora.webapi.core.domain.AppState
@accessible
trait State {
def set(v: AppState): UIO[Unit]
val get: UIO[AppState]

def get: UIO[AppState]
}

object State {
Expand Down
2 changes: 1 addition & 1 deletion webapi/src/main/scala/org/knora/webapi/core/actors/RoutingActor.scala
View file
Open in desktop
Expand Up @@ -76,7 +76,7 @@ class RoutingActor(
) extends Actor {

implicit val system: ActorSystem = context.system
val log: Logger = Logger(this.getClass())
val log: Logger = Logger(this.getClass)

/**
* The Cache Service's configuration.
Expand Down