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.

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

Move key changes into FormulaRuntime. #333

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Move key changes into FormulaRuntime. #333

merged 2 commits into from
Jan 22, 2024

Conversation

Laimiux
Copy link
Collaborator

@Laimiux Laimiux commented Jan 22, 2024

What

To remove duplication within RxJavaRuntime and FlowRuntime, I'm moving logic into shared FormulaRuntime class

@carrotkite
Copy link

carrotkite commented Jan 22, 2024

JaCoCo Code Coverage 78.72% ✅

Class Covered Meta Status
com/instacart/formula/coroutines/FlowRuntime 50% 0%
com/instacart/formula/rxjava3/RxJavaRuntime 84% 0%
com/instacart/formula/FormulaRuntime 76% 0%

Generated by 🚫 Danger

) : ManagerDelegate {
private val implementation = formula.implementation()
private var manager: FormulaManagerImpl<Input, *, Output>? = null
private var hasInitialFinished = false
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is not needed anymore - previously it was used to ensure that on formula start, we wait for all actions to start running before emitting a value. Currently, it's done within FormulaManagerImpl.

threadChecker.check("Input arrived on a wrong thread.")
if (!runtime.isKeyValid(input)) {
runtime.terminate()
runtime = runtimeFactory()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was a bit weird that we recreate the runtime if key changes. Instead, the runtime will handle state reset internally.

performTermination()
terminateManager(manager)

if (localInputId != inputId && !terminated) {

Choose a reason for hiding this comment

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

The logic here looks fine, but it's a little confusing that we have both manager.isTerminated() and a terminated flag (and that they'll have opposite values).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They could potentially both be true at the same time, will add documentation and maybe rename it.

@Laimiux Laimiux force-pushed the laimonas/cleanup branch 2 times, most recently from b27b7a3 to b0b40c7 Compare January 22, 2024 18:20
@Laimiux Laimiux merged commit c7ed4d2 into master Jan 22, 2024
3 checks passed
@Laimiux Laimiux deleted the laimonas/cleanup branch January 22, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants