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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
