Skip to content

Conversation

dvob
Copy link
Contributor

@dvob dvob commented Sep 25, 2025

When using JOSDK with Kotlin I ran into the problem that the retry did not work when the create method of a DependentResource throws an Exception not inherited from RuntimeException.

The change switches the handling from RuntimeException to Exception so that it works with Kotlin where you can throw Exceptions from methods which don't declare that they throw.

I changed this in the NodeExecutor to address the problem I specifically ran into. Then I also grepped for other places where RuntimeException is catched and then changed it in PollingEventSource as well.

Maybe there are other places (e.g. OperatorException) which should be changed.

@openshift-ci openshift-ci bot requested review from metacosm and xstefank September 25, 2025 06:57
@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

Hi @dvob I'm not against this, we probably don't want to change OperatorException. So you were able to test this PR, with properly with kotlin?

Copy link
Collaborator

@csviri csviri left a comment

Choose a reason for hiding this comment

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

I think that we should at least comment on those places that Exception is required because of kotlin

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

also created a followup issue: #2967

@dvob
Copy link
Contributor Author

dvob commented Sep 25, 2025

Theoretically this is for all JVM-languages which do not have the concept of checked vs. unchecked exceptions and not just for Kotlin. But not sure if in practice any other language is used.

Yes, I tested this with Kotlin and it fixed the problem (reconcile of DependentResource which throws Exception now causes a retry)

I will go ahead and add the comments at the places where we catch Exception, ok?
Do you want me to put this into the same commit or add a second commit and then you can squash it on merging?

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

I will go ahead and add the comments at the places where we catch Exception, ok?

Yes, pls.

Do you want me to put this into the same commit or add a second commit and then you can squash it on merging?

Does not matter; we squash commits.

Thank you!

@metacosm metacosm changed the title fix: handle all excptions to support Kotlin fix: handle all exceptions to support Kotlin Sep 25, 2025
@dvob dvob force-pushed the handle-all-exceptions branch 2 times, most recently from 565ea5a to 9b0bb8b Compare September 25, 2025 07:34
@dvob
Copy link
Contributor Author

dvob commented Sep 25, 2025

I added the comments and also fixed the typos in the commit message

Handle Exception and not just RuntimeException to support Kotlin and
probably other JVM languages which do not have the concept of checked
exceptions.
@dvob dvob force-pushed the handle-all-exceptions branch from 9b0bb8b to 49a913e Compare September 25, 2025 07:44
Copy link
Collaborator

@xstefank xstefank left a comment

Choose a reason for hiding this comment

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

I'm OK with this but shouldn't this be considered a breaking change? If someone indeed is now throwing RuntimeException it will stop working?

@csviri
Copy link
Collaborator

csviri commented Sep 25, 2025

I'm OK with this but shouldn't this be considered a breaking change? If someone indeed is now throwing RuntimeException it will stop working?

I don't see how it would stop working, could you pls elaborate?

@xstefank
Copy link
Collaborator

scratch that, forgot which exception extends which exception

@csviri csviri merged commit e368709 into operator-framework:main Sep 25, 2025
1 check failed
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.

4 participants