Migrate Blazegraph module to Cats Effect #4478
Conversation
dantb
changed the title
...fl/bluebrain/nexus/delta/plugins/blazegraph/slowqueries/BlazegraphSlowQueryLoggerSuite.scala
Outdated
Show resolved
Hide resolved
| def timed[A](io: IO[A])(implicit c: Clock[IO]): IO[(A, FiniteDuration)] = | ||
| for { | ||
| start <- c.monotonic(MILLISECONDS) | ||
| result <- io | ||
| finish <- c.monotonic(MILLISECONDS) | ||
| } yield (result, (finish - start).millis) |
There was a problem hiding this comment.
Probably worth a test
There was a problem hiding this comment.
I'm reluctant since it's essentially copied stdlib code which you wouldn't normally test... Kinda covered in those slow query tests as well
There was a problem hiding this comment.
I think copied code is still worth testing if we rely on it. Tests are there to make sure it works going forward as well as now. It should be super easy to write
| @@ -135,7 +138,7 @@ class BlazegraphClient( | |||
| count <- countAsString.value.toLongOption | |||
| } yield count | |||
|
|
|||
| IO.fromOption(count, InvalidCountRequest(index, sparqlQuery.value)) | |||
| IO.fromOption(count)(InvalidCountRequest(index, sparqlQuery.value)) | |||
There was a problem hiding this comment.
I will never forgive the Cats authors for this method signature
| val queryString = queries.map(_.value).mkString("\n") | ||
| val pss = new ParameterizedSparqlString | ||
| pss.setCommandText(queryString) | ||
| for { | ||
| _ <- IO.fromTry(Try(pss.asUpdate())).mapError(th => InvalidUpdateRequest(index, queryString, th.getMessage)) | ||
| _ <- IO(pss.asUpdate()).adaptError(e => InvalidUpdateRequest(index, queryString, e.getMessage)) |
There was a problem hiding this comment.
If .apply is the same as .delay, we probably want to decide which one to use. I know most existing code would use delay
There was a problem hiding this comment.
I'd go for apply, and not just because fewer characters 😛 it follows the same pattern as Try and Future
There was a problem hiding this comment.
Personally I like the explicit nature and consistency with .pure that .delay has. But it's probably a team decision
| .when(duration >= longQueryThreshold)(logSlowQuery(context, outcome.isLeft, duration)) | ||
| def apply[A](context: BlazegraphQueryContext, query: IO[A]): IO[A] = { | ||
| IOInstant | ||
| .timed(query.attempt) |
There was a problem hiding this comment.
I think you have an implicit class method for this now?
There was a problem hiding this comment.
In your PR or somewhere else?
There was a problem hiding this comment.
Oh I thought .timed was an implicitly added method. Maybe it could be?
| IOInstant.now | ||
| .flatTap(_ => logger.warn(s"Slow blazegraph query recorded: duration '$duration', view '${context.view}'")) |
There was a problem hiding this comment.
I realise you're just doing the same as before, but it's a bit weird to flatTap when you aren't interested in the value. Maybe just the log >> Instant.now?
...fl/bluebrain/nexus/delta/plugins/blazegraph/slowqueries/BlazegraphSlowQueryLoggerSuite.scala
Outdated
Show resolved
Hide resolved
Fixes #4432 and #4476
Sorry for the size, I started with just the client and followed the compiler - ended up doing the whole module