You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by "CritasWang (via GitHub)" <gi...@apache.org> on 2025/09/22 09:22:45 UTC
[PR] remove org.reflections:reflections, only test use it [iotdb]
CritasWang opened a new pull request, #16456:
URL: https://github.com/apache/iotdb/pull/16456
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] remove org.reflections:reflections, only test use it [iotdb]
Posted by "JackieTien97 (via GitHub)" <gi...@apache.org>.
JackieTien97 merged PR #16456:
URL: https://github.com/apache/iotdb/pull/16456
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] remove org.reflections:reflections, only test use it [iotdb]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #16456:
URL: https://github.com/apache/iotdb/pull/16456#issuecomment-3318165973
## [Codecov](https://app.codecov.io/gh/apache/iotdb/pull/16456?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 38.69%. Comparing base ([`25427da`](https://app.codecov.io/gh/apache/iotdb/commit/25427da164a886b1b72de7c1733eef990bdb9dd6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f512bfd`](https://app.codecov.io/gh/apache/iotdb/commit/f512bfde2dd62b283fc8bc72d4ae6d506f9e18bd?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #16456 +/- ##
=========================================
Coverage 38.69% 38.69%
Complexity 207 207
=========================================
Files 4934 4934
Lines 325576 325569 -7
Branches 41369 41368 -1
=========================================
+ Hits 125978 125995 +17
+ Misses 199598 199574 -24
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/iotdb/pull/16456?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] remove org.reflections:reflections, only test use it [iotdb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #16456:
URL: https://github.com/apache/iotdb/pull/16456#discussion_r2367345094
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/datastructure/visibility/VisibilityTestUtils.java:
##########
@@ -22,20 +22,14 @@
import org.apache.iotdb.commons.pipe.datastructure.result.Result;
import org.apache.iotdb.pipe.api.PipePlugin;
-import org.reflections.Reflections;
-import org.reflections.scanners.SubTypesScanner;
-
import java.util.HashMap;
import java.util.Map;
import java.util.Set;
public class VisibilityTestUtils {
- public static Result<Void, String> testVisibilityCompatibilityEntry(final String prefix) {
- // Use the Reflections library to scan the classpath
- final Reflections reflections = new Reflections(prefix, new SubTypesScanner(false));
- final Set<Class<? extends PipePlugin>> subTypes = reflections.getSubTypesOf(PipePlugin.class);
-
+ public static Result<Void, String> testVisibilityCompatibilityEntry(
+ final Set<Class<? extends PipePlugin>> subTypes) {
// Create root node
final TreeNode root = new TreeNode(PipePlugin.class);
Review Comment:
This is a breaking change to a public method in a main-scope class. If VisibilityTestUtils is intended for tests only, consider moving it to test sources (or a test-jar) or reducing visibility (e.g., package-private) and documenting it as test-only; alternatively, provide a deprecated overload to preserve binary compatibility for any existing callers.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/schemaregion/mtree/loader/MNodeFactoryLoader.java:
##########
@@ -87,10 +87,7 @@ public IMNodeFactory<IMemMNode> getMemMNodeIMNodeFactory() {
@SuppressWarnings("squid:S3740")
private IMNodeFactory loadMNodeFactory(Class<?> nodeType) {
- Reflections reflections =
- new Reflections(
- new ConfigurationBuilder().forPackages(scanPackages.toArray(new String[0])));
- Set<Class<?>> nodeFactorySet = reflections.getTypesAnnotatedWith(MNodeFactory.class);
+
for (Class<?> nodeFactory : nodeFactorySet) {
if (isGenericMatch(nodeFactory, nodeType) && isEnvMatch(nodeFactory, env)) {
try {
Review Comment:
nodeFactorySet is a mutable HashSet exposed via the public addNodeFactory method and iterated without synchronization; concurrent mutation can cause ConcurrentModificationException. Make nodeFactorySet final and immutable after construction (e.g., populate in the constructor and wrap with Collections.unmodifiableSet), or use a thread-safe set such as CopyOnWriteArraySet.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/schemaregion/mtree/loader/MNodeFactoryLoader.java:
##########
@@ -47,14 +44,17 @@ public class MNodeFactoryLoader {
@SuppressWarnings("java:S3077")
private volatile IMNodeFactory<IMemMNode> memMNodeIMNodeFactory;
+ private Set<Class<?>> nodeFactorySet;
+
private MNodeFactoryLoader() {
- addScanPackage("org.apache.iotdb.db.schemaengine.schemaregion.mtree.impl.pbtree.mnode.factory");
- addScanPackage("org.apache.iotdb.db.schemaengine.schemaregion.mtree.impl.mem.mnode.factory");
+ nodeFactorySet = new HashSet<>();
+ addNodeFactory(MemMNodeFactory.class);
+ addNodeFactory(CacheMNodeFactory.class);
setEnv(SchemaConstant.DEFAULT_MNODE_FACTORY_ENV);
}
- public void addScanPackage(String scanPackage) {
- scanPackages.add(scanPackage);
+ public void addNodeFactory(Class<?> clazz) {
Review Comment:
If external code is not expected to mutate the registry at runtime, consider restricting addNodeFactory visibility (e.g., package-private) or removing it to prevent unsupported runtime reconfiguration. Otherwise, document its thread-safety and lifecycle expectations.
```suggestion
void addNodeFactory(Class<?> clazz) {
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/schemaregion/mtree/loader/MNodeFactoryLoader.java:
##########
@@ -47,14 +44,17 @@ public class MNodeFactoryLoader {
@SuppressWarnings("java:S3077")
private volatile IMNodeFactory<IMemMNode> memMNodeIMNodeFactory;
+ private Set<Class<?>> nodeFactorySet;
+
private MNodeFactoryLoader() {
- addScanPackage("org.apache.iotdb.db.schemaengine.schemaregion.mtree.impl.pbtree.mnode.factory");
- addScanPackage("org.apache.iotdb.db.schemaengine.schemaregion.mtree.impl.mem.mnode.factory");
+ nodeFactorySet = new HashSet<>();
+ addNodeFactory(MemMNodeFactory.class);
+ addNodeFactory(CacheMNodeFactory.class);
setEnv(SchemaConstant.DEFAULT_MNODE_FACTORY_ENV);
}
- public void addScanPackage(String scanPackage) {
- scanPackages.add(scanPackage);
+ public void addNodeFactory(Class<?> clazz) {
+ nodeFactorySet.add(clazz);
}
Review Comment:
nodeFactorySet is a mutable HashSet exposed via the public addNodeFactory method and iterated without synchronization; concurrent mutation can cause ConcurrentModificationException. Make nodeFactorySet final and immutable after construction (e.g., populate in the constructor and wrap with Collections.unmodifiableSet), or use a thread-safe set such as CopyOnWriteArraySet.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/schemaregion/mtree/loader/MNodeFactoryLoader.java:
##########
@@ -47,14 +44,17 @@ public class MNodeFactoryLoader {
@SuppressWarnings("java:S3077")
private volatile IMNodeFactory<IMemMNode> memMNodeIMNodeFactory;
+ private Set<Class<?>> nodeFactorySet;
+
private MNodeFactoryLoader() {
- addScanPackage("org.apache.iotdb.db.schemaengine.schemaregion.mtree.impl.pbtree.mnode.factory");
- addScanPackage("org.apache.iotdb.db.schemaengine.schemaregion.mtree.impl.mem.mnode.factory");
+ nodeFactorySet = new HashSet<>();
+ addNodeFactory(MemMNodeFactory.class);
+ addNodeFactory(CacheMNodeFactory.class);
Review Comment:
Hardcoding factory classes reduces extensibility and requires code changes for new implementations. Consider switching to Java's ServiceLoader to discover IMNodeFactory providers without Reflections (keep production free of classpath scanning) and then filter with isGenericMatch/isEnvMatch.
##########
.github/workflows/pipe-it.yml:
##########
@@ -49,7 +49,7 @@ jobs:
- name: Set up JDK ${{ matrix.java }}
uses: actions/setup-java@v4
with:
- distribution: liberica
+ distribution: oracle
Review Comment:
Workflows now mix Oracle (most jobs) and Corretto (compile-check) distributions; consider standardizing the JDK distribution across workflows to avoid environment-dependent build/test differences.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/schemaengine/schemaregion/SchemaRegionLoader.java:
##########
@@ -48,13 +47,8 @@ public class SchemaRegionLoader {
@SuppressWarnings("unchecked")
public SchemaRegionLoader() {
- Reflections reflections =
- new Reflections(
- new ConfigurationBuilder()
- .forPackages(PACKAGE_NAME)
- .filterInputsBy(new FilterBuilder().includePackage(PACKAGE_NAME)));
-
- Set<Class<?>> annotatedSchemaRegionSet = reflections.getTypesAnnotatedWith(SchemaRegion.class);
+ Set<Class<?>> annotatedSchemaRegionSet =
+ new HashSet<>(Arrays.asList(SchemaRegionMemoryImpl.class, SchemaRegionPBTreeImpl.class));
Review Comment:
Replacing discovery with a hardcoded set restricts pluggability; new SchemaRegion implementations will be ignored until this list is updated. To keep runtime free of Reflections while preserving extensibility, consider using ServiceLoader<ISchemaRegion> (with META-INF/services) and then filtering by the @SchemaRegion annotation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] remove org.reflections:reflections, only test use it [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #16456:
URL: https://github.com/apache/iotdb/pull/16456#issuecomment-3317908601
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=16456) **Quality Gate failed**
Failed conditions
 [B Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=16456) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=16456)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] remove org.reflections:reflections, only test use it [iotdb]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #16456:
URL: https://github.com/apache/iotdb/pull/16456#issuecomment-3318226520
## [](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=16456) **Quality Gate failed**
Failed conditions
 [B Maintainability Rating on New Code](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=16456) (required ≥ A)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_iotdb&pullRequest=16456)
##
 Catch issues before they fail your Quality Gate with our IDE extension  [SonarQube for IDE](https://www.sonarsource.com/products/sonarlint/features/connected-mode/?referrer=pull-request)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org