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: use ServiceLoader to find implementations instead of searching classes in jars #5885

Merged
merged 1 commit into from
May 11, 2023

Conversation

vlsi
Copy link
Collaborator

@vlsi vlsi commented May 7, 2023

Description

ServiceLoader is Java standard approach for locating implementaitons, and it allows pluggability without relying on a filesystem layout.

The change is backward-compatible, so JMeter searches implementations via ServiceLoader, and then it scans the jars like it was doing previously. However, all the JMeter-provided implementations would be provided as services.

Fixes #5883
See discussion https://lists.apache.org/thread/4t7wcjh45q61v300j1954c8l8931swrr

TODO:

  • add JMeterUtils#loadServices and tests
  • migrate JMeterUtils#findClassesThatExtend usages to ServiceLoader
  • migrate ClassFinder#findClassesThatExtend usages to ServiceLoader
  • deprecate ClassFinder methods
  • Add Jar manifest entry JMeter-Skip-Class-Scanning: true so ClassFinder ignores JMeter jars
  • Add javadocs when appropriate
  • Add changelog entry
  • fix The Kotlin Gradle plugin was loaded multiple times in different subprojects warning

Out of scope of the PR:

  • Split JMeterGUIComponent into metadata and UI parts #5895
    migrate MenuFactory from ClassFinder#findClassesThatExtend
    MenuFactory is tricky as ServiceLoader instantiates services by default, so we can't use it for heavy-weight objects. MenuFactory makes extra efforts to avoid creating all the GUI components, so we should suggest an alternative.
  • migrate ActionRouter. The code there is involved, and it is not clear why it searches for classes several times (see 574ceb6), implemented in b15439e

Motivation and Context

The current JMeterUtils#findClassesThatExtend and ClassFinder.findClassesThatExtend impose issues:

  1. They expect classes to be packaged in jars. It is inconvenient for testing purposes where the dependencies might come form classes folders
  2. Class scanning takes time. The current workaround is to use String contains, String notContains parameters, however, it does not resolve the issue, and it makes an awkward API overall
  3. It will make it easier to create tests. For instance, I want to use __counter function for feat: add property to disable FunctionProperty caching #5788 in :src:core test module, however, ClassFinder does not work there as it wants jar files. After migration to ServiceLoader, I could add testRuntimeOnly(projects.src.functions) dependency, and then CompoundVariable would be able to locate functions with a ServiceLoader.

@vlsi vlsi force-pushed the service_loader branch from 6d3bb73 to 7643ada Compare May 7, 2023 13:54
@vlsi
Copy link
Collaborator Author

vlsi commented May 7, 2023

@FSchumacher , @emilianbold, do you remember the reasons for 574ceb6 by chance?

UPD: I found the discussion: https://lists.apache.org/thread/sqj1nv0kcls1smy23gyow35fsx51v7sh

@vlsi
Copy link
Collaborator Author

vlsi commented May 10, 2023

It looks like the extra overhead for annotation processing is negligible, so I think we can live with it.

I've tried ./gradlew testClasses --rerun-tasks --no-build-cache, and it looks like the difference does not exceed 7 seconds.

with annotation processing

BUILD SUCCESSFUL in 1m 36s
197 actionable tasks: 197 executed

https://ge.apache.org/s/td4x7pa3gnddm

before this PR

BUILD SUCCESSFUL in 1m 29s
186 actionable tasks: 186 executed

https://ge.apache.org/s/wevuaxhuqoonw

@vlsi vlsi force-pushed the service_loader branch from ef24844 to 5b79071 Compare May 10, 2023 17:55
@vlsi
Copy link
Collaborator Author

vlsi commented May 10, 2023

I think the PR is good to go (if the tests pass). I will merge it soon provided there are no objections.

@vlsi vlsi added this to the 5.6 milestone May 10, 2023
@milamberspace
Copy link
Contributor

@vlsi you want release 5.5.1 with xalan security fix before merge this PR? Or we prepare the next release 5.6 or 6.0?

@vlsi
Copy link
Collaborator Author

vlsi commented May 10, 2023

I am afraid we have too many changes anyway on the main branch.

At the same time, I think Xalan is not the only security fix we need to implement.

Do you suggest creating a bugfix branch with xalan only changes?

I suggest polishing what we have and releasing it as 5.6.
I do not expect it to have major breakages or major breakthroughs, however, we have updated many dependencies, and we fixed several annoying bugs, so 5.6 sounds a good version for me.

…classes in jars

ServiceLoader is Java standard approach for locating implementaitons,
and it allows pluggability without relying on a filesystem layout.

Fixes apache#5883
@Mingun
Copy link

Mingun commented Oct 14, 2024

@vlsi, you're crazy man. Why all the changes in one commit? How to understand which interfaces supports new annotation?

@vlsi
Copy link
Collaborator Author

vlsi commented Oct 14, 2024

@Mingun , please check the tutorial update which comes within the PR: https://github.com/apache/jmeter/pull/5885/files#diff-576fa49214b48be798a06fe0b81dbd6b0276fa58b0c46fd081c278bab7f00a63R183-R184

      The interfaces that are supported for service loading are marked with
      <code>org.apache.jorphan.reflect.JMeterService</code>
      annotation.

@Mingun
Copy link

Mingun commented Oct 14, 2024

The problem is finding those interfaces. It's not pretty easy to find them in 186 changed files. I just cloned repository and find all occurrences of JMeterService (search in GH not always return all results so I was not sure that it is show me all occurrences).

@vlsi
Copy link
Collaborator Author

vlsi commented Oct 14, 2024

Try find usages for JMeterService in the IDE.

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.

Use ServiceLoader to find implementations instead of ClassFinder
3 participants