dant3 commented on code in PR #7753:
URL: https://github.com/apache/ignite-3/pull/7753#discussion_r2918776913
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/CompoundModule.java:
##########
@@ -37,11 +37,45 @@ public class CompoundModule implements ConfigurationModule {
private final ConfigurationType type;
private final List<ConfigurationModule> modules;
- public CompoundModule(ConfigurationType type,
Collection<ConfigurationModule> modules) {
+ private CompoundModule(ConfigurationType type,
Collection<ConfigurationModule> modules) {
this.type = type;
this.modules = List.copyOf(modules);
}
+ /**
+ * Creates a compound local {@link ConfigurationModule} from the given
collection.
+ *
+ * @param modules All configuration modules to filter.
+ * @return A compound module containing only {@link
ConfigurationType#LOCAL} modules.
+ */
+ public static ConfigurationModule local(Collection<ConfigurationModule>
modules) {
+ return ofType(ConfigurationType.LOCAL, modules);
+ }
+
+ /**
+ * Creates a compound distributed {@link ConfigurationModule} from the
given collection.
+ *
+ * @param modules All configuration modules to filter.
+ * @return A compound module containing only {@link
ConfigurationType#DISTRIBUTED} modules.
+ */
+ public static ConfigurationModule
distributed(Collection<ConfigurationModule> modules) {
+ return ofType(ConfigurationType.DISTRIBUTED, modules);
+ }
+
+ /**
+ * Creates a compound {@link ConfigurationModule} of the specified type
from the given collection.
+ *
+ * @param type The configuration type to filter by.
+ * @param modules All configuration modules to filter.
+ * @return A compound module containing only modules of the given type.
+ */
+ public static ConfigurationModule ofType(ConfigurationType type,
Collection<ConfigurationModule> modules) {
Review Comment:
Shoult it be private instead? Or at lease package-private? Looks like an
implementation detail factory-constructor.
##########
migration-tools/modules/migration-tools-commons/src/main/java/org/apache/ignite/migrationtools/config/Ignite3ConfigurationUtils.java:
##########
@@ -65,7 +68,7 @@ public static CombinedConfigRegistry
loadCombinedRegistry(Path nodeCfgPath, Path
* @param includeDefaults Include defaults.
*/
public static ConfigurationRegistry loadNodeConfiguration(Path cfgPath,
boolean includeDefaults) {
- return loadConfigurations(cfgPath,
ConfigurationModules.create(null).local(), includeDefaults);
+ return loadConfigurations(cfgPath,
CompoundModule.local(ConfigurationModule.loadAll(null)), includeDefaults);
Review Comment:
We are loading all modules from ClassLoader using ServiceLoader at least
twice. Shouldn't we cache it?
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -52,6 +57,26 @@
* @see PolymorphicConfig
*/
public interface ConfigurationModule {
+ /**
+ * Loads all {@link ConfigurationModule}s from the classpath using the
provided class loader.
+ *
+ * @param classLoader The class loader to use, or {@code null} to use the
default class loader.
+ * @return All configuration modules found on the classpath.
+ * @throws IllegalStateException If no configuration modules are found.
+ */
+ static List<ConfigurationModule> loadAll(@Nullable ClassLoader
classLoader) {
Review Comment:
This code is a public api (since its in `configuration-api`), and loading of
modules with the ServiceLoader seems like an implementation details, so it's
probably its worth to move it into `configuration` module.
--
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]
