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

Refactor: using Reflections to load AbstractCollect #1681

Closed
wants to merge 1 commit into from

Conversation

Calvin979
Copy link
Contributor

@Calvin979 Calvin979 commented Mar 24, 2024

What's changed?

Hertzbeat uses SPI to load AbstractCollect, which means anybody who intends to add a new monitor needs to modify SPI file(in the path META-INF\services) as well.
Therefore I use Reflections to replace SPI. It will scan concrete class of AbstractCollect and create instance automatically.

New Dependency

<dependency>
        <groupId>org.reflections</groupId>
        <artifactId>reflections</artifactId>
        <version>0.10.2</version>
</dependency>

Checklist

  • I have read the Contributing Guide
  • I have written the necessary doc or comment.
  • I have added the necessary unit tests and all cases have passed.

Add or update API

  • I have added the necessary e2e tests and all cases have passed.

@tomsun28
Copy link
Contributor

👍👍 Great Idea!
I see we use the org.reflections to reflection, but this package not maintained anymore. ronmamo/reflections#465
And we will build the graalvm native-image later for hertzbeat, It seems that this package does not support it well. ronmamo/reflections#470

@tomsun28 tomsun28 added enhancement New feature or request good first pull request Good for newcomers new feature labels Mar 24, 2024
@Calvin979
Copy link
Contributor Author

👍👍 Great Idea! I see we use the org.reflections to reflection, but this package not maintained anymore. ronmamo/reflections#465 And we will build the graalvm native-image later for hertzbeat, It seems that this package does not support it well. ronmamo/reflections#470

Yeah, looks like it doesn't work on graalvm native-image. Should I close this pr?

@tomsun28
Copy link
Contributor

Yeah, looks like it doesn't work on graalvm native-image. Should I close this pr?

We can keep it now and see how to modify it later, or wait until we make a native image.

@Calvin979
Copy link
Contributor Author

Yeah, looks like it doesn't work on graalvm native-image. Should I close this pr?

We can keep it now and see how to modify it later, or wait until we make a native image.

Hi, We can use applicationContext.getBeansOfType if org.reflections doesn't work on graalvm native-image.
image
image

@gcdd1993
Copy link
Contributor

建议尽量少用反射,SPI配置文件应该不是特别大的负担 @Calvin979 @tomsun28

@hertzbeat
Copy link
Contributor

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


It is recommended to use reflection as little as possible. The SPI configuration file should not be a particularly big burden @Calvin979 @tomsun28

@tomsun28
Copy link
Contributor

okk, try not to use reflection in higher versions of jdk

@Calvin979 Calvin979 closed this Apr 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first pull request Good for newcomers new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants