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

Disable auto refresh on feature installation #5153

Merged
merged 1 commit into from Aug 8, 2023

Conversation

JulianKniephoff
Copy link
Member

Due to some wiring inconsistencies (and not all Opencast bundles being able to hot-restart properly) this currently breaks feature installation, because Karaf weants to refresh javax.persistence, which more or less everything depends on.

/cc #5151

@JulianKniephoff JulianKniephoff added ELAN Pull requests originating from ELAN e.V. quick review A pull request that can be reviewed quickly labels Aug 3, 2023
@lkiesow
Copy link
Member

lkiesow commented Aug 7, 2023

What does this pull request do and why? You just described a problem of Opencast. How does disabling auto refresh on feature installations help, and what side effect does it have?

@JulianKniephoff
Copy link
Member Author

JulianKniephoff commented Aug 7, 2023

Is there anything in particular that's unclear? Because I feel like most of your question is answered in the title and description: This PR disables Karaf's auto-refresh of bundles on feature installation, because said auto-refresh currently breaks Opencast when you install a feature. I would like to be able to install a feature (the Annotation Tool in particular, but that shouldn't matter), so disabling this would allow me to do so.

You can also skip auto-refresh using feature:install -r/--no-auto-refresh, but there is no way to skip it for features you drop into deploy, for example, which I would like to do. Plus: With the current settings you can't install features at all rn, so this is strictly an improvement, IMO. 🤷‍♀️

Side-note: Our plugin system is based on Karaf features, and lo and behold, the plugin manager does this as well.

As for the side-effects: That's fair. So what does auto-refresh even do? It rewires bundles, which you might want to do after a feature installation because the feature could contain new bundles/services/... that satisfy preexisting requirements. So the side-effect of this PR is obviously that this doesn't happen, which could lead to inconsistencies in your wiring, I think, but I'm not 100% sure. There is a whole swath of use cases where this does not apply, though. The Annotation Tool as an additional module that the rest of the system is completely agnostic of is one such example, apparently all of our plugins are, too.

And again, this strictly enables more stuff, only breaks in cases that didn't even work/exist before (if at all), and can be reverted if/when this is fixed in Karaf.

@lkiesow
Copy link
Member

lkiesow commented Aug 7, 2023

Huh, I wrote that plugin stuff 😁
But that doesn't mean I remember what Karaf's auto-refresh does.
Thanks for the explanation. That's what was missing for me.

@lkiesow
Copy link
Member

lkiesow commented Aug 7, 2023

Is this intended for develop? Or do you want this fix to go into a release branch?

@JulianKniephoff
Copy link
Member Author

Fair. 😁 Sorry if that came across a bit salty. 🧂

Re. branch: For me personally it would be useful in the release branches as well, but this is a behavior changing config change, so I'm not sure if it's allowed to go in there? On the other hand, as said, the behavior it changes was broken to begin with, soooo ... 🤔🤷‍♀️?

@lkiesow lkiesow self-assigned this Aug 8, 2023
Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

Seems reasonable and seems to work.
I don't care about the branch.
I'll see where it ends up when I merge it after the technical meeting.

Due to some wiring inconsistencies (and not all Opencast bundles
being able to hot-restart properly) this currently breaks
feature installation, because Karaf weants to refresh
`javax.persistence`, which more or less everything depends on.

/cc opencast#5151
@JulianKniephoff JulianKniephoff changed the base branch from develop to r/13.x August 8, 2023 13:12
@JulianKniephoff
Copy link
Member Author

JulianKniephoff commented Aug 8, 2023

In that case: Rebased on and retargeted to r/13.x. 😊👍

EDIT: The build fails due to the corrupted jar issue now, sadly. ☹️

@lkiesow lkiesow merged commit 384ae61 into opencast:r/13.x Aug 8, 2023
1 of 3 checks passed
@JulianKniephoff JulianKniephoff deleted the disable-autorefresh branch September 11, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ELAN Pull requests originating from ELAN e.V. quick review A pull request that can be reviewed quickly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants