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

Feat: Eliminate ServiceLoader in JoinerSupport #696

Closed
marinier opened this issue Mar 8, 2024 · 2 comments · Fixed by #705
Closed

Feat: Eliminate ServiceLoader in JoinerSupport #696

marinier opened this issue Mar 8, 2024 · 2 comments · Fixed by #705
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@marinier
Copy link
Contributor

marinier commented Mar 8, 2024

Is your feature request related to a problem? Please describe.
As described in #664, my code needs to specify the class loader that the ServiceLoader should use. I was able to do this for ScoreDirectorFactoryFactory in #690 because a class loader was already configurable and passed into the right place -- I just had to use it.

JoinerSupport also uses ServiceLoader, but it is not configurable. @triceo commented here that the use of ServiceLoader is purely historical and that it would be nice to remove it. This issue is to perform that removal.

Describe the solution you'd like
The simplest approach would be to change JoinerSupport::getJoinerService to just directly instantiate DefaultJoinerService without going through ServiceLoader.

A more thorough removal would update Joiners to directly instantiate DefaultJoinerService (as a member variable) and use it, eliminating JoinerSupport entirely.

Both of these approaches leave the JoinerService interface, but if we really want to close the door on having different JoinerServices in the future, we could get rid of the interface and just have the single implementation.

If one of these approaches is ok, I'll create a PR for it.

Describe alternatives you've considered
Another possibility would be to allow a class loader to be set on JoinerSupport. This would leave the ServiceLoader mechanism in place but allow it to work in cases where the default class loader does not work. To do this we could add a new static method setClassLoader, and users would need to know to call it before getJoinerService could be called.

This is perhaps a simpler solution than removing the ServiceLoader, but it feels like a hack. In particular, if the timing of the call to setClassLoader is not early enough, the class loader won't actually be used but this may not be obvious to the user resulting in very confusing bugs. We could add a warning (or even exception) if setClassLoader is called after INSTANCE is set. But if the use of ServiceLoader here is purely historical, then it would be nice to just eliminate it entirely.

Additional context
N/A

@marinier marinier added enhancement New feature or request process/needs triage Requires initial assessment of validity, priority etc. labels Mar 8, 2024
@triceo
Copy link
Contributor

triceo commented Mar 8, 2024

@marinier Your plan sounds reasonable and we appreciate your offer to help. We discussed the topic earlier today and decided to go a bit further than what your plan is.

Together with the ServiceLoader not being necessary anymore, the core-impl JAR and the constraint-streams JAR are no longer necessary for that same reason. Therefore the full scope of the work will include merging all of that code back to the core JAR, along with removing all the boilerplate that was necessary for the ServiceLoader stuff to work.

Considering that this will be quite a surgery, I'm going to work on it myself. If all goes well, it will be ready for the April release of Timefold Solver. (Unfortunately not in time for Timefold Solver 1.8.0 next week.)

@marinier
Copy link
Contributor Author

marinier commented Mar 8, 2024

Ok, thank you for taking this on! Waiting until April will be well worth it!

@triceo triceo self-assigned this Mar 8, 2024
@triceo triceo removed the process/needs triage Requires initial assessment of validity, priority etc. label Mar 8, 2024
@triceo triceo added this to the v1.9.0 milestone Mar 8, 2024
@triceo triceo linked a pull request Mar 11, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants