add finalize to NadelTransform - #693
Conversation
| if (isSkipIncludeSpecialField(field) && ((transform as NadelTransform<*>) !is NadelSkipIncludeTransform)) { | ||
| null | ||
| } else { | ||
| val executionTransformContext = if (transformContexts.containsKey(transform)) { |
There was a problem hiding this comment.
just
transformContexts.getOrPut(transform) {...
without if (transformContexts.containsKey(transform)) { is not enough, we need to check if a key was already added with the null value.
map.getOrPut(k) { fn() } runs fn() every time when k: null was added to the map. so we check if there is an explicit (transform: null) mapping in the map already
| ) | ||
| } | ||
| } | ||
| finalizeJobs.awaitAll() |
There was a problem hiding this comment.
If there's no result then launch { is better of async { and .joinAll() instead of .awaitAll()
But also I think if you use this coroutineContext it will only exit the lambda once all the child coroutines are done, so you don't actually need .joinAll()
e.g. this
suspend fun main() {
coroutineScope {
List(3) {
launch {
println("Start")
delay(1.seconds)
println("Ok")
}
}
}
println("Hello")
}
Prints
Start
Start
Start
Ok
Ok
Ok
Hello
So just coroutineScope { contextByTransform.forEach { launch { <work> } } } should be sufficient.
| val queryTransformTimingStep: NadelInstrumentationTimingParameters.Step, | ||
| val resultTransformTimingStep: NadelInstrumentationTimingParameters.Step, | ||
| val state: T, | ||
| val transformServiceExecutionContext: NadelTransformServiceExecutionContext?, |
There was a problem hiding this comment.
I feel like we should just have a Map<NadelTransform, NadelTransformServiceExecutionContext> instead of storing this in every Step
| val executionSteps: MutableMap<ExecutableNormalizedField, List<NadelExecutionPlan.Step<Any>>> = | ||
| mutableMapOf() | ||
|
|
||
| val transformContexts: MutableMap<NadelTransform<Any>, NadelTransformServiceExecutionContext?> = |
There was a problem hiding this comment.
Yeah I feel like we should pass this into NadelExecutionPlan instead of storing it in every Step
But either way nbd let's fix that later
|
@gnawf thanks, I will address in a follow up PR |
2 participants
Please make sure you consider the following: