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

Make a dynamic OSGi dependency static #5670

Merged

Conversation

JulianKniephoff
Copy link
Member

I don't see any reason for it to be dynamic, neither do I see an easy way to not have it in the system.

@JulianKniephoff JulianKniephoff added maintenance This pull request is addressing maintenance issues ELAN Pull requests originating from ELAN e.V. java Pull requests that update Java code possibly controversial labels Mar 13, 2024
@JulianKniephoff JulianKniephoff changed the base branch from develop to r/15.x March 13, 2024 18:12
I don't see any reason for it to be dynamic,
neither do I see an easy way to **not** have it in the system.
@gregorydlogan
Copy link
Member

IIRC this breaks in clustered environments, but I'd have to boot those up and check.

@JulianKniephoff
Copy link
Member Author

Ooh interesting, I didn't consider that. Will try to look into it.

@JulianKniephoff
Copy link
Member Author

The rationale seems to be this: #4736

Although I still don't fully understand how that happens and/or how the corresponding PR fixes it. 🤔 Maybe someone can educate me here.

But my thinking is: conductor runs on admin{presentation}, where you have search-service-remote no matter what, even in the scenario @JamesUoM paints, at least as far as I understand it. So SearchUpdatedEventHandler will bind to SearchServiceRemoteImpl, will be activated, and ConductingSeriesUpdatedEventHandler will bind to it in turn, no? 🤔 And then I would (still) expect it to breakif the remote can't reach a proper impl.

@gregorydlogan
Copy link
Member

Ugh, I can see how it fixes it, but I can also see why we had such buggy behaviour in those classes. Basically James' fix was to do exactly what you're doing here, except if an event handler fires and no search service is bound then the handler just drops the event that it should pass on the floor and walks away. I... probably should not have merged that as it was haha.

In this case you're making it a hard dependency, and you're right in that your change either starts properly (and hopefully works), or it just doesn't start at all. In my testing it seems to start, although I haven't actually tested proper cluster operation. I wouldn't expect that part to break though, since you haven't changed anything there. In the case of a remote being active but not being able to find/contact an impl you'd just see a hang here - it would wait eternally for its loverimpl to come back.

I'd say this is reasonable. And I suspect my recollections of this kind of thing causing issues are from old (12.x-ish) version where enough of the arch has changed that I'm no longer relevant.

@JamesUoM
Copy link
Contributor

My original intention was to remove a hard dependency on the Search service, as I feel that all publication channels should be "optional", though Search is the default it should be replacable with any of the alternatives.

We had achieved the removal of Search in our current deployment but it turned out there were other hard dependencies in 12.x and I'm running an "unused" Search service, so on its own by change didn't achieve what I intended.

If my change is causing issues then please revert it - I'm not sure I ever understood what the conductor was actually doing. The "hardcoded" dependency on Search is something I would like to revisit.

@gregorydlogan gregorydlogan self-assigned this Mar 27, 2024
@gregorydlogan gregorydlogan merged commit 7f0e6db into opencast:r/15.x Mar 27, 2024
3 checks passed
@JulianKniephoff JulianKniephoff deleted the static-update-handler branch April 30, 2024 14:04
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. java Pull requests that update Java code maintenance This pull request is addressing maintenance issues possibly controversial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants