You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/08/18 07:19:14 UTC
[GitHub] [ignite-3] tkalkirill opened a new pull request #290: IGNITE-15047 Internal configuration
tkalkirill opened a new pull request #290:
URL: https://github.com/apache/ignite-3/pull/290
https://issues.apache.org/jira/browse/IGNITE-15047
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699017444
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
What about `ConverterConfigToMapObject ` ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699066412
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
I think it's ok.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r698570089
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -99,25 +124,24 @@ public ConfigurationRegistry(
return cgen.instantiateNode(rootKey.schemaClass());
}
};
- }
- /** {@inheritDoc} */
- @Override public void start() {
rootKeys.forEach(rootKey -> {
- cgen.compileRootSchema(rootKey.schemaClass());
+ cgen.compileRootSchema(rootKey.schemaClass(), extensions);
DynamicConfiguration<?, ?> cfg = cgen.instantiateCfg(rootKey, changer);
configs.put(rootKey.key(), cfg);
});
+ }
+ /** {@inheritDoc} */
+ @Override public void start() {
changer.start();
}
/** {@inheritDoc} */
@Override public void stop() {
- if (changer != null)
- changer.stop();
+ changer.stop();
Review comment:
Since the **changer** has become `final`.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
I need a function that returns two objects, but your proposal will only have one.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
What about `ConverterConfigToMapObject ` ?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
+ .map(ParameterizedType::typeFromJavaClassName)
+ .toArray(ParameterizedType[]::new);
+
// Node class definition.
ClassDefinition classDef = new ClassDefinition(
of(PUBLIC, FINAL),
internalName(schemaClassInfo.nodeClassName),
type(InnerNode.class),
- typeFromJavaClassName(schemaClassInfo.viewClassName),
- typeFromJavaClassName(schemaClassInfo.changeClassName)
+ interfaces
);
- // Spec field.
- FieldDefinition specField = classDef.declareField(of(PRIVATE, FINAL), "_spec", schemaClass);
+ // Spec fields.
+ Map<Class<?>, FieldDefinition> specFields = new HashMap<>();
- // org.apache.ignite.internal.configuration.tree.InnerNode#schemaType
- addNodeSchemaTypeMethod(classDef, specField);
+ int i = 0;
- // Cached array of reflected fields. Helps to avoid unnecessary clonings.
- Field[] schemaFields = schemaClass.getDeclaredFields();
+ for (Class<?> schemaCls : union(schemaExtensions, schemaClass))
+ specFields.put(schemaCls, classDef.declareField(of(PRIVATE, FINAL), "_spec" + i++, schemaCls));
Review comment:
I suggest leaving it as it is.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -431,26 +460,27 @@ else if (isConfigValue(schemaField))
/**
* Implements default constructor for the node class. It initializes {@code _spec} field and every other field
* that represents named list configuration.
+ *
* @param classDef Node class definition.
- * @param schemaClass Configuration Schema class.
- * @param specField Field definition for the {@code _spec} field of the node class.
- * @param schemaFields Configuration Schema class fields.
+ * @param specFields Definition of fields for the {@code _spec#} fields of the node class.
+ * Mapping: configuration schema class -> {@code _spec#} field.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Field definitions for all fields of node class excluding {@code _spec}.
*/
private void addNodeConstructor(
ClassDefinition classDef,
- Class<?> schemaClass,
- FieldDefinition specField,
- Field[] schemaFields,
+ Map<Class<?>, FieldDefinition> specFields,
+ Set<Field> schemaFields,
Map<String, FieldDefinition> fieldDefs
) {
MethodDefinition ctor = classDef.declareConstructor(of(PUBLIC));
// super();
ctor.getBody().append(ctor.getThis()).invokeConstructor(InnerNode.class);
- // this._spec = new MyConfigurationSchema();
- ctor.getBody().append(ctor.getThis().setField(specField, newInstance(schemaClass)));
+ // this._spec# = new MyConfigurationSchema();
+ for (Map.Entry<Class<?>, FieldDefinition> e : specFields.entrySet())
+ ctor.getBody().append(ctor.getThis().setField(e.getValue(), newInstance(e.getKey())));
Review comment:
It looks like optimization, I propose to do it in a separate ticket.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
I think it's ok.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699018149
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
+ .map(ParameterizedType::typeFromJavaClassName)
+ .toArray(ParameterizedType[]::new);
+
// Node class definition.
ClassDefinition classDef = new ClassDefinition(
of(PUBLIC, FINAL),
internalName(schemaClassInfo.nodeClassName),
type(InnerNode.class),
- typeFromJavaClassName(schemaClassInfo.viewClassName),
- typeFromJavaClassName(schemaClassInfo.changeClassName)
+ interfaces
);
- // Spec field.
- FieldDefinition specField = classDef.declareField(of(PRIVATE, FINAL), "_spec", schemaClass);
+ // Spec fields.
+ Map<Class<?>, FieldDefinition> specFields = new HashMap<>();
- // org.apache.ignite.internal.configuration.tree.InnerNode#schemaType
- addNodeSchemaTypeMethod(classDef, specField);
+ int i = 0;
- // Cached array of reflected fields. Helps to avoid unnecessary clonings.
- Field[] schemaFields = schemaClass.getDeclaredFields();
+ for (Class<?> schemaCls : union(schemaExtensions, schemaClass))
+ specFields.put(schemaCls, classDef.declareField(of(PRIVATE, FINAL), "_spec" + i++, schemaCls));
Review comment:
I suggest leaving it as it is.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov merged pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
ibessonov merged pull request #290:
URL: https://github.com/apache/ignite-3/pull/290
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r698335587
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -106,15 +114,87 @@ private boolean process0(RoundEnvironment roundEnvironment) {
String packageName = elementPackage.getQualifiedName().toString();
// Find all the fields of the schema
- List<VariableElement> fields = clazz.getEnclosedElements().stream()
- .filter(el -> el.getKind() == ElementKind.FIELD)
- .map(VariableElement.class::cast)
- .collect(Collectors.toList());
+ Collection<VariableElement> fields = fields(clazz);
ConfigurationRoot rootAnnotation = clazz.getAnnotation(ConfigurationRoot.class);
- // Is root of the configuration
- boolean isRoot = rootAnnotation != null;
+ // Is root of the configuration.
+ boolean isRootConfig = rootAnnotation != null;
+
+ // Is the configuration.
+ boolean isConfig = clazz.getAnnotation(Config.class) != null;
+
+ // Is the internal configuration.
+ boolean isInternalConfig = clazz.getAnnotation(InternalConfiguration.class) != null;
+
+ if (isInternalConfig) {
Review comment:
I think we should extract it into a separate method
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -76,15 +83,33 @@
* @param rootKeys Configuration root keys.
* @param validators Validators.
* @param storage Configuration storage.
- * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type.
+ * @param internalSchemaExtensions Internal extensions ({@link InternalConfiguration})
+ * of configuration schemas ({@link ConfigurationRoot} and {@link Config}).
+ * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type,
+ * or if the schema or its extensions are not valid.
*/
public ConfigurationRegistry(
Collection<RootKey<?, ?>> rootKeys,
Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators,
- ConfigurationStorage storage
+ ConfigurationStorage storage,
+ Collection<Class<?>> internalSchemaExtensions
) {
checkConfigurationType(rootKeys, storage);
+ Set<Class<?>> allSchemas = collectSchemas(rootKeys.stream().map(RootKey::schemaClass).collect(toSet()));
+
+ Map<Class<?>, Set<Class<?>>> extensions = internalSchemaExtensions(internalSchemaExtensions);
+
+ Set<Class<?>> notInAllSchemas = extensions.keySet().stream()
+ .filter(not(allSchemas::contains))
+ .collect(toSet());
+
+ if (!notInAllSchemas.isEmpty()) {
Review comment:
We can replace the condition with "allSchemas.containsAll(extensions.keySet())" and calculate the exact set only if condition is met. I think the code will become a little bit simpler after that.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -99,25 +124,24 @@ public ConfigurationRegistry(
return cgen.instantiateNode(rootKey.schemaClass());
}
};
- }
- /** {@inheritDoc} */
- @Override public void start() {
rootKeys.forEach(rootKey -> {
- cgen.compileRootSchema(rootKey.schemaClass());
+ cgen.compileRootSchema(rootKey.schemaClass(), extensions);
DynamicConfiguration<?, ?> cfg = cgen.instantiateCfg(rootKey, changer);
configs.put(rootKey.key(), cfg);
});
+ }
+ /** {@inheritDoc} */
+ @Override public void start() {
changer.start();
}
/** {@inheritDoc} */
@Override public void stop() {
- if (changer != null)
- changer.stop();
+ changer.stop();
Review comment:
Why was this "if" even a thing?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -160,18 +184,19 @@ public void initializeDefaults() {
Object node;
try {
- node = ConfigurationUtil.find(path, superRoot);
+ node = ConfigurationUtil.find(path, superRoot, false);
}
catch (KeyNotFoundException e) {
throw new IllegalArgumentException(e.getMessage());
}
if (node instanceof TraversableTreeNode)
return ((TraversableTreeNode)node).accept(null, visitor);
+ else {
Review comment:
What's the point of "else"? Old code looked fine to me :)
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
Not the best name, don't you think so?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
Review comment:
Should these strings manipulations be extracted to a more appropriate place?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
Review comment:
Could be Stream.concat and then "distinct()", you don't have to allocate all these collections, right?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
+ .map(ParameterizedType::typeFromJavaClassName)
+ .toArray(ParameterizedType[]::new);
+
// Node class definition.
ClassDefinition classDef = new ClassDefinition(
of(PUBLIC, FINAL),
internalName(schemaClassInfo.nodeClassName),
type(InnerNode.class),
- typeFromJavaClassName(schemaClassInfo.viewClassName),
- typeFromJavaClassName(schemaClassInfo.changeClassName)
+ interfaces
);
- // Spec field.
- FieldDefinition specField = classDef.declareField(of(PRIVATE, FINAL), "_spec", schemaClass);
+ // Spec fields.
+ Map<Class<?>, FieldDefinition> specFields = new HashMap<>();
- // org.apache.ignite.internal.configuration.tree.InnerNode#schemaType
- addNodeSchemaTypeMethod(classDef, specField);
+ int i = 0;
- // Cached array of reflected fields. Helps to avoid unnecessary clonings.
- Field[] schemaFields = schemaClass.getDeclaredFields();
+ for (Class<?> schemaCls : union(schemaExtensions, schemaClass))
+ specFields.put(schemaCls, classDef.declareField(of(PRIVATE, FINAL), "_spec" + i++, schemaCls));
Review comment:
Can we generate a better name? The one that would make sense while debugging?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
+ .filter(cls -> Stream.of(cls.getDeclaredFields()).anyMatch(f -> schemaFieldName.equals(f.getName())))
+ .map(cls -> prefix(cls) + CHANGE_CLASS_POSTFIX)
Review comment:
String magic should be in a separate method, as I mentioned earlier
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
+ .filter(cls -> Stream.of(cls.getDeclaredFields()).anyMatch(f -> schemaFieldName.equals(f.getName())))
Review comment:
I don't like that this complex operation is in the loop, where's that old map from Field to Set of Classes?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -431,26 +460,27 @@ else if (isConfigValue(schemaField))
/**
* Implements default constructor for the node class. It initializes {@code _spec} field and every other field
* that represents named list configuration.
+ *
* @param classDef Node class definition.
- * @param schemaClass Configuration Schema class.
- * @param specField Field definition for the {@code _spec} field of the node class.
- * @param schemaFields Configuration Schema class fields.
+ * @param specFields Definition of fields for the {@code _spec#} fields of the node class.
+ * Mapping: configuration schema class -> {@code _spec#} field.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Field definitions for all fields of node class excluding {@code _spec}.
*/
private void addNodeConstructor(
ClassDefinition classDef,
- Class<?> schemaClass,
- FieldDefinition specField,
- Field[] schemaFields,
+ Map<Class<?>, FieldDefinition> specFields,
+ Set<Field> schemaFields,
Map<String, FieldDefinition> fieldDefs
) {
MethodDefinition ctor = classDef.declareConstructor(of(PUBLIC));
// super();
ctor.getBody().append(ctor.getThis()).invokeConstructor(InnerNode.class);
- // this._spec = new MyConfigurationSchema();
- ctor.getBody().append(ctor.getThis().setField(specField, newInstance(schemaClass)));
+ // this._spec# = new MyConfigurationSchema();
+ for (Map.Entry<Class<?>, FieldDefinition> e : specFields.entrySet())
+ ctor.getBody().append(ctor.getThis().setField(e.getValue(), newInstance(e.getKey())));
Review comment:
I remember asking you to make initialization of these fields lazy, can you please do it for me? I also expect "copy()" to preserve already instantiated values
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -588,83 +621,97 @@ private void addNodeChangeMethod(
}
/**
- * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildrenMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
Review comment:
Can I safely assume that this method could have parameters like "schemaFields" and "extensionsFields" and you decided to merge them together, only to split them back later? This definitely makes the code more complicated
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java
##########
@@ -29,89 +29,127 @@
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor) {
+ * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor, boolean includeInternal) {
* visitor.visitInnerNode("pojoField1", this.pojoField1);
*
* visitor.visitNamedListNode("pojoField2", this.pojoField2);
*
* visitor.visitLeafNode("primitiveField1", this.primitiveField1);
*
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * if(includeInternal) {
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * }
* }
* </code></pre>
*
* Order of fields must be the same as they are described in configuration schema.
*
* @param visitor Configuration visitor.
+ * @param includeInternal Include internal configuration nodes (private configuration extensions).
* @param <T> Parameter type of the passed visitor.
*/
- public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor);
+ public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor, boolean includeInternal);
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor) throws NoSuchElementException {
- * switch (key) {
- * case "pojoField1":
- * visitor.visitInnerNode("pojoField1", this.pojoField1);
- * break;
+ * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor, boolean includeInternal) throws NoSuchElementException {
+ * if(boolean includeInternal) {
+ * switch (key) {
+ * case "pojoField1":
+ * visitor.visitInnerNode("pojoField1", this.pojoField1);
+ * break;
*
- * case "pojoField2":
- * visitor.visitNamedListNode("pojoField2", this.pojoField2);
- * break;
+ * case "pojoField2":
+ * visitor.visitNamedListNode("pojoField2", this.pojoField2);
+ * break;
*
- * case "primitiveField1":
- * visitor.visitLeafNode("primitiveField1", this.primitiveField1);
- * break;
+ * case "primitiveField1":
+ * visitor.visitLeafNode("primitiveField1", this.primitiveField1);
+ * break;
*
- * case "primitiveField2":
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
- * break;
+ * case "primitiveField2":
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * break;
*
- * default:
- * throw new NoSuchElementException(key);
+ * default:
+ * throw new NoSuchElementException(key);
+ * }
+ * }
+ * else {
+ * switch (key) {
+ * case "primitiveField2":
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * break;
+ * default:
+ * throw new NoSuchElementException(key);
+ * }
* }
* }
* </code></pre>
*
* @param key Name of the child.
* @param visitor Configuration visitor.
+ * @param includeInternal Include internal configuration nodes (private configuration extensions).
* @param <T> Parameter type of passed visitor.
* @return Whatever {@code visitor} returned.
* @throws NoSuchElementException If field {@code key} is not found.
*/
- public abstract <T> T traverseChild(String key, ConfigurationVisitor<T> visitor) throws NoSuchElementException;
+ public abstract <T> T traverseChild(
+ String key,
+ ConfigurationVisitor<T> visitor,
+ boolean includeInternal
+ ) throws NoSuchElementException;
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public abstract void construct(String key, ConfigurationSource src) throws NoSuchElementException {
+ * {@literal @}Override public abstract void construct(String key, ConfigurationSource src, boolean includeInternal) throws NoSuchElementException {
+ * if(includeInternal) {
Review comment:
```suggestion
* if (includeInternal) {
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -185,7 +201,8 @@
private final Map<Class<?>, SchemaClassesInfo> schemasInfo = new HashMap<>();
/** Class generator instance. */
- private final ClassGenerator generator = ClassGenerator.classGenerator(this.getClass().getClassLoader());
+ private final ClassGenerator generator = ClassGenerator.classGenerator(this.getClass().getClassLoader())
+ .dumpClassFilesTo(new File("C:\\test").toPath());
Review comment:
Dude, this should be removed from the code :)
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
Looks too complicated. I suppose that one of fields whould look like this:
BiFunction<String, Boolean, InnerNode> nodeCreator;
What do you think? Are there ways to avoid tuples?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
+ .filter(cls -> Stream.of(cls.getDeclaredFields()).anyMatch(f -> schemaFieldName.equals(f.getName())))
+ .map(cls -> prefix(cls) + CHANGE_CLASS_POSTFIX)
+ .forEach(changeCls -> addNodeChangeMethod(classDef, schemaField, fieldDef, changeCls, true));
Review comment:
No no no, this should be a separate method, whose implementation just delegates everything to a non-bridged method, right? But I see that bridge method have their own implementations right now with identical bodies. Not cool
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java
##########
@@ -29,89 +29,127 @@
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor) {
+ * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor, boolean includeInternal) {
* visitor.visitInnerNode("pojoField1", this.pojoField1);
*
* visitor.visitNamedListNode("pojoField2", this.pojoField2);
*
* visitor.visitLeafNode("primitiveField1", this.primitiveField1);
*
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * if(includeInternal) {
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * }
* }
* </code></pre>
*
* Order of fields must be the same as they are described in configuration schema.
*
* @param visitor Configuration visitor.
+ * @param includeInternal Include internal configuration nodes (private configuration extensions).
* @param <T> Parameter type of the passed visitor.
*/
- public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor);
+ public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor, boolean includeInternal);
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor) throws NoSuchElementException {
- * switch (key) {
- * case "pojoField1":
- * visitor.visitInnerNode("pojoField1", this.pojoField1);
- * break;
+ * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor, boolean includeInternal) throws NoSuchElementException {
+ * if(boolean includeInternal) {
Review comment:
```suggestion
* if (boolean includeInternal) {
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java
##########
@@ -29,89 +29,127 @@
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor) {
+ * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor, boolean includeInternal) {
* visitor.visitInnerNode("pojoField1", this.pojoField1);
*
* visitor.visitNamedListNode("pojoField2", this.pojoField2);
*
* visitor.visitLeafNode("primitiveField1", this.primitiveField1);
*
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * if(includeInternal) {
Review comment:
```suggestion
* if (includeInternal) {
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -588,83 +621,97 @@ private void addNodeChangeMethod(
}
/**
- * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildrenMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
+ Map<String, FieldDefinition> fieldDefs,
+ Set<Class<?>> schemaExtensions
) {
MethodDefinition traverseChildrenMtd = classDef.declareMethod(
of(PUBLIC),
"traverseChildren",
type(void.class),
- arg("visitor", type(ConfigurationVisitor.class))
+ arg("visitor", type(ConfigurationVisitor.class)),
+ arg("includeInternal", type(boolean.class))
).addException(NoSuchElementException.class);
+ Variable includeInternalVar = traverseChildrenMtd.getScope().getVariable("includeInternal");
+
BytecodeBlock mtdBody = traverseChildrenMtd.getBody();
- for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ if (schemaExtensions.isEmpty())
+ invokeVisitForTraverseChildren(schemaFields, fieldDefs, traverseChildrenMtd).forEach(mtdBody::append);
+ else {
+ // {@code true} - internal, {@code false} - public.
+ Map<Boolean, List<Field>> fields = schemaFields.stream()
+ .collect(partitioningBy(f -> schemaExtensions.contains(f.getDeclaringClass())));
+
+ invokeVisitForTraverseChildren(fields.get(false), fieldDefs, traverseChildrenMtd).forEach(mtdBody::append);
+
+ BytecodeBlock includeInternalBlock = new BytecodeBlock();
+
+ invokeVisitForTraverseChildren(fields.get(true), fieldDefs, traverseChildrenMtd)
+ .forEach(includeInternalBlock::append);
- // Visit every field. Returned value isn't used so we pop it off the stack.
- mtdBody.append(invokeVisit(traverseChildrenMtd, schemaField, fieldDef).pop());
+ mtdBody.append(new IfStatement().condition(includeInternalVar).ifTrue(includeInternalBlock));
}
mtdBody.ret();
}
/**
- * Implements {@link InnerNode#traverseChild(String, ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChild(String, ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
Review comment:
I guess same complain applies to other methods as well
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -588,83 +621,97 @@ private void addNodeChangeMethod(
}
/**
- * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildrenMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
+ Map<String, FieldDefinition> fieldDefs,
+ Set<Class<?>> schemaExtensions
) {
MethodDefinition traverseChildrenMtd = classDef.declareMethod(
of(PUBLIC),
"traverseChildren",
type(void.class),
- arg("visitor", type(ConfigurationVisitor.class))
+ arg("visitor", type(ConfigurationVisitor.class)),
+ arg("includeInternal", type(boolean.class))
).addException(NoSuchElementException.class);
+ Variable includeInternalVar = traverseChildrenMtd.getScope().getVariable("includeInternal");
+
BytecodeBlock mtdBody = traverseChildrenMtd.getBody();
- for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ if (schemaExtensions.isEmpty())
+ invokeVisitForTraverseChildren(schemaFields, fieldDefs, traverseChildrenMtd).forEach(mtdBody::append);
+ else {
+ // {@code true} - internal, {@code false} - public.
+ Map<Boolean, List<Field>> fields = schemaFields.stream()
Review comment:
This place in particular, I don't like it
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -1149,4 +1167,154 @@ private static String capitalize(String name) {
return boxed == null ? clazz : boxed;
}
+
+ /**
+ * Create bytecode blocks that invokes of {@link ConfigurationVisitor}'s methods for
+ * {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)}.
+ *
+ * @param schemaFields Fields of the schema.
+ * @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param traverseChildrenMtd Method definition {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)}
+ * defined in {@code *Node} class.
+ * @return Created bytecode blocks that invokes of {@link ConfigurationVisitor}'s methods for fields.
+ */
+ private Collection<BytecodeNode> invokeVisitForTraverseChildren(
+ Collection<Field> schemaFields,
+ Map<String, FieldDefinition> fieldDefs,
+ MethodDefinition traverseChildrenMtd
+ ) {
+ if (schemaFields.isEmpty())
+ return List.of();
+ else {
+ List<BytecodeNode> res = new ArrayList<>(schemaFields.size());
+
+ for (Field schemaField : schemaFields) {
Review comment:
Why not stream?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
Review comment:
BTW, change interfaces would be enough, given that they extend view interfaces already!
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
Why do you need two objects in the result? How do you plan to use them?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
Ok, I see, can we at least use Tuple<Boolean, InnerNode>?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
I'd prefer having "Visitor" suffix, what about "ConverterToMapVisitor" or something similar?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699012580
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
I need a function that returns two objects, but your proposal will only have one.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r698335587
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/Processor.java
##########
@@ -106,15 +114,87 @@ private boolean process0(RoundEnvironment roundEnvironment) {
String packageName = elementPackage.getQualifiedName().toString();
// Find all the fields of the schema
- List<VariableElement> fields = clazz.getEnclosedElements().stream()
- .filter(el -> el.getKind() == ElementKind.FIELD)
- .map(VariableElement.class::cast)
- .collect(Collectors.toList());
+ Collection<VariableElement> fields = fields(clazz);
ConfigurationRoot rootAnnotation = clazz.getAnnotation(ConfigurationRoot.class);
- // Is root of the configuration
- boolean isRoot = rootAnnotation != null;
+ // Is root of the configuration.
+ boolean isRootConfig = rootAnnotation != null;
+
+ // Is the configuration.
+ boolean isConfig = clazz.getAnnotation(Config.class) != null;
+
+ // Is the internal configuration.
+ boolean isInternalConfig = clazz.getAnnotation(InternalConfiguration.class) != null;
+
+ if (isInternalConfig) {
Review comment:
I think we should extract it into a separate method
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -76,15 +83,33 @@
* @param rootKeys Configuration root keys.
* @param validators Validators.
* @param storage Configuration storage.
- * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type.
+ * @param internalSchemaExtensions Internal extensions ({@link InternalConfiguration})
+ * of configuration schemas ({@link ConfigurationRoot} and {@link Config}).
+ * @throws IllegalArgumentException If the configuration type of the root keys is not equal to the storage type,
+ * or if the schema or its extensions are not valid.
*/
public ConfigurationRegistry(
Collection<RootKey<?, ?>> rootKeys,
Map<Class<? extends Annotation>, Set<Validator<? extends Annotation, ?>>> validators,
- ConfigurationStorage storage
+ ConfigurationStorage storage,
+ Collection<Class<?>> internalSchemaExtensions
) {
checkConfigurationType(rootKeys, storage);
+ Set<Class<?>> allSchemas = collectSchemas(rootKeys.stream().map(RootKey::schemaClass).collect(toSet()));
+
+ Map<Class<?>, Set<Class<?>>> extensions = internalSchemaExtensions(internalSchemaExtensions);
+
+ Set<Class<?>> notInAllSchemas = extensions.keySet().stream()
+ .filter(not(allSchemas::contains))
+ .collect(toSet());
+
+ if (!notInAllSchemas.isEmpty()) {
Review comment:
We can replace the condition with "allSchemas.containsAll(extensions.keySet())" and calculate the exact set only if condition is met. I think the code will become a little bit simpler after that.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -99,25 +124,24 @@ public ConfigurationRegistry(
return cgen.instantiateNode(rootKey.schemaClass());
}
};
- }
- /** {@inheritDoc} */
- @Override public void start() {
rootKeys.forEach(rootKey -> {
- cgen.compileRootSchema(rootKey.schemaClass());
+ cgen.compileRootSchema(rootKey.schemaClass(), extensions);
DynamicConfiguration<?, ?> cfg = cgen.instantiateCfg(rootKey, changer);
configs.put(rootKey.key(), cfg);
});
+ }
+ /** {@inheritDoc} */
+ @Override public void start() {
changer.start();
}
/** {@inheritDoc} */
@Override public void stop() {
- if (changer != null)
- changer.stop();
+ changer.stop();
Review comment:
Why was this "if" even a thing?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -160,18 +184,19 @@ public void initializeDefaults() {
Object node;
try {
- node = ConfigurationUtil.find(path, superRoot);
+ node = ConfigurationUtil.find(path, superRoot, false);
}
catch (KeyNotFoundException e) {
throw new IllegalArgumentException(e.getMessage());
}
if (node instanceof TraversableTreeNode)
return ((TraversableTreeNode)node).accept(null, visitor);
+ else {
Review comment:
What's the point of "else"? Old code looked fine to me :)
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
Not the best name, don't you think so?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
Review comment:
Should these strings manipulations be extracted to a more appropriate place?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
Review comment:
Could be Stream.concat and then "distinct()", you don't have to allocate all these collections, right?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
+ .map(ParameterizedType::typeFromJavaClassName)
+ .toArray(ParameterizedType[]::new);
+
// Node class definition.
ClassDefinition classDef = new ClassDefinition(
of(PUBLIC, FINAL),
internalName(schemaClassInfo.nodeClassName),
type(InnerNode.class),
- typeFromJavaClassName(schemaClassInfo.viewClassName),
- typeFromJavaClassName(schemaClassInfo.changeClassName)
+ interfaces
);
- // Spec field.
- FieldDefinition specField = classDef.declareField(of(PRIVATE, FINAL), "_spec", schemaClass);
+ // Spec fields.
+ Map<Class<?>, FieldDefinition> specFields = new HashMap<>();
- // org.apache.ignite.internal.configuration.tree.InnerNode#schemaType
- addNodeSchemaTypeMethod(classDef, specField);
+ int i = 0;
- // Cached array of reflected fields. Helps to avoid unnecessary clonings.
- Field[] schemaFields = schemaClass.getDeclaredFields();
+ for (Class<?> schemaCls : union(schemaExtensions, schemaClass))
+ specFields.put(schemaCls, classDef.declareField(of(PRIVATE, FINAL), "_spec" + i++, schemaCls));
Review comment:
Can we generate a better name? The one that would make sense while debugging?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
+ .filter(cls -> Stream.of(cls.getDeclaredFields()).anyMatch(f -> schemaFieldName.equals(f.getName())))
+ .map(cls -> prefix(cls) + CHANGE_CLASS_POSTFIX)
Review comment:
String magic should be in a separate method, as I mentioned earlier
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
+ .filter(cls -> Stream.of(cls.getDeclaredFields()).anyMatch(f -> schemaFieldName.equals(f.getName())))
Review comment:
I don't like that this complex operation is in the loop, where's that old map from Field to Set of Classes?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -431,26 +460,27 @@ else if (isConfigValue(schemaField))
/**
* Implements default constructor for the node class. It initializes {@code _spec} field and every other field
* that represents named list configuration.
+ *
* @param classDef Node class definition.
- * @param schemaClass Configuration Schema class.
- * @param specField Field definition for the {@code _spec} field of the node class.
- * @param schemaFields Configuration Schema class fields.
+ * @param specFields Definition of fields for the {@code _spec#} fields of the node class.
+ * Mapping: configuration schema class -> {@code _spec#} field.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Field definitions for all fields of node class excluding {@code _spec}.
*/
private void addNodeConstructor(
ClassDefinition classDef,
- Class<?> schemaClass,
- FieldDefinition specField,
- Field[] schemaFields,
+ Map<Class<?>, FieldDefinition> specFields,
+ Set<Field> schemaFields,
Map<String, FieldDefinition> fieldDefs
) {
MethodDefinition ctor = classDef.declareConstructor(of(PUBLIC));
// super();
ctor.getBody().append(ctor.getThis()).invokeConstructor(InnerNode.class);
- // this._spec = new MyConfigurationSchema();
- ctor.getBody().append(ctor.getThis().setField(specField, newInstance(schemaClass)));
+ // this._spec# = new MyConfigurationSchema();
+ for (Map.Entry<Class<?>, FieldDefinition> e : specFields.entrySet())
+ ctor.getBody().append(ctor.getThis().setField(e.getValue(), newInstance(e.getKey())));
Review comment:
I remember asking you to make initialization of these fields lazy, can you please do it for me? I also expect "copy()" to preserve already instantiated values
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -588,83 +621,97 @@ private void addNodeChangeMethod(
}
/**
- * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildrenMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
Review comment:
Can I safely assume that this method could have parameters like "schemaFields" and "extensionsFields" and you decided to merge them together, only to split them back later? This definitely makes the code more complicated
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java
##########
@@ -29,89 +29,127 @@
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor) {
+ * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor, boolean includeInternal) {
* visitor.visitInnerNode("pojoField1", this.pojoField1);
*
* visitor.visitNamedListNode("pojoField2", this.pojoField2);
*
* visitor.visitLeafNode("primitiveField1", this.primitiveField1);
*
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * if(includeInternal) {
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * }
* }
* </code></pre>
*
* Order of fields must be the same as they are described in configuration schema.
*
* @param visitor Configuration visitor.
+ * @param includeInternal Include internal configuration nodes (private configuration extensions).
* @param <T> Parameter type of the passed visitor.
*/
- public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor);
+ public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor, boolean includeInternal);
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor) throws NoSuchElementException {
- * switch (key) {
- * case "pojoField1":
- * visitor.visitInnerNode("pojoField1", this.pojoField1);
- * break;
+ * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor, boolean includeInternal) throws NoSuchElementException {
+ * if(boolean includeInternal) {
+ * switch (key) {
+ * case "pojoField1":
+ * visitor.visitInnerNode("pojoField1", this.pojoField1);
+ * break;
*
- * case "pojoField2":
- * visitor.visitNamedListNode("pojoField2", this.pojoField2);
- * break;
+ * case "pojoField2":
+ * visitor.visitNamedListNode("pojoField2", this.pojoField2);
+ * break;
*
- * case "primitiveField1":
- * visitor.visitLeafNode("primitiveField1", this.primitiveField1);
- * break;
+ * case "primitiveField1":
+ * visitor.visitLeafNode("primitiveField1", this.primitiveField1);
+ * break;
*
- * case "primitiveField2":
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
- * break;
+ * case "primitiveField2":
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * break;
*
- * default:
- * throw new NoSuchElementException(key);
+ * default:
+ * throw new NoSuchElementException(key);
+ * }
+ * }
+ * else {
+ * switch (key) {
+ * case "primitiveField2":
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * break;
+ * default:
+ * throw new NoSuchElementException(key);
+ * }
* }
* }
* </code></pre>
*
* @param key Name of the child.
* @param visitor Configuration visitor.
+ * @param includeInternal Include internal configuration nodes (private configuration extensions).
* @param <T> Parameter type of passed visitor.
* @return Whatever {@code visitor} returned.
* @throws NoSuchElementException If field {@code key} is not found.
*/
- public abstract <T> T traverseChild(String key, ConfigurationVisitor<T> visitor) throws NoSuchElementException;
+ public abstract <T> T traverseChild(
+ String key,
+ ConfigurationVisitor<T> visitor,
+ boolean includeInternal
+ ) throws NoSuchElementException;
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public abstract void construct(String key, ConfigurationSource src) throws NoSuchElementException {
+ * {@literal @}Override public abstract void construct(String key, ConfigurationSource src, boolean includeInternal) throws NoSuchElementException {
+ * if(includeInternal) {
Review comment:
```suggestion
* if (includeInternal) {
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -185,7 +201,8 @@
private final Map<Class<?>, SchemaClassesInfo> schemasInfo = new HashMap<>();
/** Class generator instance. */
- private final ClassGenerator generator = ClassGenerator.classGenerator(this.getClass().getClassLoader());
+ private final ClassGenerator generator = ClassGenerator.classGenerator(this.getClass().getClassLoader())
+ .dumpClassFilesTo(new File("C:\\test").toPath());
Review comment:
Dude, this should be removed from the code :)
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
Looks too complicated. I suppose that one of fields whould look like this:
BiFunction<String, Boolean, InnerNode> nodeCreator;
What do you think? Are there ways to avoid tuples?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -322,59 +367,43 @@ private ClassDefinition createNodeClass(Class<?> schemaClass) {
}
// Constructor.
- addNodeConstructor(classDef, schemaClass, specField, schemaFields, fieldDefs);
+ addNodeConstructor(classDef, specFields, schemaFields, fieldDefs);
// VIEW and CHANGE methods.
for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ String schemaFieldName = schemaField.getName();
+
+ FieldDefinition fieldDef = fieldDefs.get(schemaFieldName);
addNodeViewMethod(classDef, schemaField, fieldDef);
- addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo);
+ // Add change methods.
+
+ // At the beginning, a method that returns the implementation of all (Change) interfaces.
+ addNodeChangeMethod(classDef, schemaField, fieldDef, schemaClassInfo.nodeClassName, false);
+
+ // Bridges for each (Change) interface.
+ union(schemaExtensions, schemaClass).stream()
+ .filter(cls -> Stream.of(cls.getDeclaredFields()).anyMatch(f -> schemaFieldName.equals(f.getName())))
+ .map(cls -> prefix(cls) + CHANGE_CLASS_POSTFIX)
+ .forEach(changeCls -> addNodeChangeMethod(classDef, schemaField, fieldDef, changeCls, true));
Review comment:
No no no, this should be a separate method, whose implementation just delegates everything to a non-bridged method, right? But I see that bridge method have their own implementations right now with identical bodies. Not cool
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java
##########
@@ -29,89 +29,127 @@
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor) {
+ * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor, boolean includeInternal) {
* visitor.visitInnerNode("pojoField1", this.pojoField1);
*
* visitor.visitNamedListNode("pojoField2", this.pojoField2);
*
* visitor.visitLeafNode("primitiveField1", this.primitiveField1);
*
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * if(includeInternal) {
+ * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * }
* }
* </code></pre>
*
* Order of fields must be the same as they are described in configuration schema.
*
* @param visitor Configuration visitor.
+ * @param includeInternal Include internal configuration nodes (private configuration extensions).
* @param <T> Parameter type of the passed visitor.
*/
- public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor);
+ public abstract <T> void traverseChildren(ConfigurationVisitor<T> visitor, boolean includeInternal);
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor) throws NoSuchElementException {
- * switch (key) {
- * case "pojoField1":
- * visitor.visitInnerNode("pojoField1", this.pojoField1);
- * break;
+ * {@literal @}Override public void traverseChild(String key, ConfigurationVisitor visitor, boolean includeInternal) throws NoSuchElementException {
+ * if(boolean includeInternal) {
Review comment:
```suggestion
* if (boolean includeInternal) {
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java
##########
@@ -29,89 +29,127 @@
/**
* Method with auto-generated implementation. Must look like this:
* <pre><code>
- * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor) {
+ * {@literal @}Override public void traverseChildren(ConfigurationVisitor visitor, boolean includeInternal) {
* visitor.visitInnerNode("pojoField1", this.pojoField1);
*
* visitor.visitNamedListNode("pojoField2", this.pojoField2);
*
* visitor.visitLeafNode("primitiveField1", this.primitiveField1);
*
- * visitor.visitLeafNode("primitiveField2", this.primitiveField2);
+ * if(includeInternal) {
Review comment:
```suggestion
* if (includeInternal) {
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -588,83 +621,97 @@ private void addNodeChangeMethod(
}
/**
- * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildrenMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
+ Map<String, FieldDefinition> fieldDefs,
+ Set<Class<?>> schemaExtensions
) {
MethodDefinition traverseChildrenMtd = classDef.declareMethod(
of(PUBLIC),
"traverseChildren",
type(void.class),
- arg("visitor", type(ConfigurationVisitor.class))
+ arg("visitor", type(ConfigurationVisitor.class)),
+ arg("includeInternal", type(boolean.class))
).addException(NoSuchElementException.class);
+ Variable includeInternalVar = traverseChildrenMtd.getScope().getVariable("includeInternal");
+
BytecodeBlock mtdBody = traverseChildrenMtd.getBody();
- for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ if (schemaExtensions.isEmpty())
+ invokeVisitForTraverseChildren(schemaFields, fieldDefs, traverseChildrenMtd).forEach(mtdBody::append);
+ else {
+ // {@code true} - internal, {@code false} - public.
+ Map<Boolean, List<Field>> fields = schemaFields.stream()
+ .collect(partitioningBy(f -> schemaExtensions.contains(f.getDeclaringClass())));
+
+ invokeVisitForTraverseChildren(fields.get(false), fieldDefs, traverseChildrenMtd).forEach(mtdBody::append);
+
+ BytecodeBlock includeInternalBlock = new BytecodeBlock();
+
+ invokeVisitForTraverseChildren(fields.get(true), fieldDefs, traverseChildrenMtd)
+ .forEach(includeInternalBlock::append);
- // Visit every field. Returned value isn't used so we pop it off the stack.
- mtdBody.append(invokeVisit(traverseChildrenMtd, schemaField, fieldDef).pop());
+ mtdBody.append(new IfStatement().condition(includeInternalVar).ifTrue(includeInternalBlock));
}
mtdBody.ret();
}
/**
- * Implements {@link InnerNode#traverseChild(String, ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChild(String, ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
Review comment:
I guess same complain applies to other methods as well
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -588,83 +621,97 @@ private void addNodeChangeMethod(
}
/**
- * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor)} method.
+ * Implements {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)} method.
+ *
* @param classDef Class definition.
- * @param schemaFields Array of all schema fields that represent configurations.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param schemaExtensions Internal extensions of the configuration schema.
*/
private void addNodeTraverseChildrenMethod(
ClassDefinition classDef,
- Field[] schemaFields,
- Map<String, FieldDefinition> fieldDefs
+ Set<Field> schemaFields,
+ Map<String, FieldDefinition> fieldDefs,
+ Set<Class<?>> schemaExtensions
) {
MethodDefinition traverseChildrenMtd = classDef.declareMethod(
of(PUBLIC),
"traverseChildren",
type(void.class),
- arg("visitor", type(ConfigurationVisitor.class))
+ arg("visitor", type(ConfigurationVisitor.class)),
+ arg("includeInternal", type(boolean.class))
).addException(NoSuchElementException.class);
+ Variable includeInternalVar = traverseChildrenMtd.getScope().getVariable("includeInternal");
+
BytecodeBlock mtdBody = traverseChildrenMtd.getBody();
- for (Field schemaField : schemaFields) {
- FieldDefinition fieldDef = fieldDefs.get(schemaField.getName());
+ if (schemaExtensions.isEmpty())
+ invokeVisitForTraverseChildren(schemaFields, fieldDefs, traverseChildrenMtd).forEach(mtdBody::append);
+ else {
+ // {@code true} - internal, {@code false} - public.
+ Map<Boolean, List<Field>> fields = schemaFields.stream()
Review comment:
This place in particular, I don't like it
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -1149,4 +1167,154 @@ private static String capitalize(String name) {
return boxed == null ? clazz : boxed;
}
+
+ /**
+ * Create bytecode blocks that invokes of {@link ConfigurationVisitor}'s methods for
+ * {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)}.
+ *
+ * @param schemaFields Fields of the schema.
+ * @param fieldDefs Definitions for all fields in {@code schemaFields}.
+ * @param traverseChildrenMtd Method definition {@link InnerNode#traverseChildren(ConfigurationVisitor, boolean)}
+ * defined in {@code *Node} class.
+ * @return Created bytecode blocks that invokes of {@link ConfigurationVisitor}'s methods for fields.
+ */
+ private Collection<BytecodeNode> invokeVisitForTraverseChildren(
+ Collection<Field> schemaFields,
+ Map<String, FieldDefinition> fieldDefs,
+ MethodDefinition traverseChildrenMtd
+ ) {
+ if (schemaFields.isEmpty())
+ return List.of();
+ else {
+ List<BytecodeNode> res = new ArrayList<>(schemaFields.size());
+
+ for (Field schemaField : schemaFields) {
Review comment:
Why not stream?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -291,26 +318,44 @@ public synchronized void compileRootSchema(Class<?> rootSchemaClass) {
}
}
- private ClassDefinition createNodeClass(Class<?> schemaClass) {
+ /**
+ * Construct a {@link InnerNode} definition for a configuration schema.
+ *
+ * @param schemaClass Configuration schema class.
+ * @param schemaFields Fields of the schema and its extensions.
+ * @param schemaExtensions Internal extensions of the configuration schema.
+ * @return Constructed {@link InnerNode} definition for the configuration schema.
+ */
+ private ClassDefinition createNodeClass(
+ Class<?> schemaClass,
+ Set<Field> schemaFields,
+ Set<Class<?>> schemaExtensions
+ ) {
SchemaClassesInfo schemaClassInfo = schemasInfo.get(schemaClass);
+ ParameterizedType[] interfaces = union(schemaExtensions, schemaClass).stream()
+ .flatMap(cls -> Stream.of(prefix(cls) + VIEW_CLASS_POSTFIX, prefix(cls) + CHANGE_CLASS_POSTFIX))
Review comment:
BTW, change interfaces would be enough, given that they extend view interfaces already!
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699046167
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
Why do you need two objects in the result? How do you plan to use them?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699049893
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConfigurationVisitorImpl.java
##########
@@ -29,18 +29,26 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import com.typesafe.config.ConfigValue;
-import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
-import org.apache.ignite.internal.configuration.tree.InnerNode;
-import org.apache.ignite.internal.configuration.tree.NamedListNode;
/**
* {@link ConfigurationVisitor} implementation that converts a configuration tree to a combination of {@link Map} and
- * {@link List} objects to convert the result into HOCON {@link ConfigValue} instance.
+ * {@link List} objects.
*/
-class HoconConfigurationVisitor implements ConfigurationVisitor<Object> {
+public class ConfigurationVisitorImpl implements ConfigurationVisitor<Object> {
Review comment:
I'd prefer having "Visitor" suffix, what about "ConverterToMapVisitor" or something similar?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699049240
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/SuperRoot.java
##########
@@ -28,109 +26,120 @@
import org.apache.ignite.internal.configuration.tree.ConfigurationSource;
import org.apache.ignite.internal.configuration.tree.ConfigurationVisitor;
import org.apache.ignite.internal.configuration.tree.InnerNode;
+import org.apache.ignite.lang.IgniteBiTuple;
+import org.jetbrains.annotations.Nullable;
-/** */
+/**
+ * Holder of root configurations.
+ */
public final class SuperRoot extends InnerNode {
- /** */
- private final SortedMap<String, InnerNode> roots = new TreeMap<>();
+ /** Root configurations. Mapping: {@link RootKey#key} -> key : configuration. */
+ private final SortedMap<String, IgniteBiTuple<RootKey<?, ?>, InnerNode>> roots = new TreeMap<>();
Review comment:
Ok, I see, can we at least use Tuple<Boolean, InnerNode>?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r699019105
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java
##########
@@ -431,26 +460,27 @@ else if (isConfigValue(schemaField))
/**
* Implements default constructor for the node class. It initializes {@code _spec} field and every other field
* that represents named list configuration.
+ *
* @param classDef Node class definition.
- * @param schemaClass Configuration Schema class.
- * @param specField Field definition for the {@code _spec} field of the node class.
- * @param schemaFields Configuration Schema class fields.
+ * @param specFields Definition of fields for the {@code _spec#} fields of the node class.
+ * Mapping: configuration schema class -> {@code _spec#} field.
+ * @param schemaFields Fields of the schema and its extensions.
* @param fieldDefs Field definitions for all fields of node class excluding {@code _spec}.
*/
private void addNodeConstructor(
ClassDefinition classDef,
- Class<?> schemaClass,
- FieldDefinition specField,
- Field[] schemaFields,
+ Map<Class<?>, FieldDefinition> specFields,
+ Set<Field> schemaFields,
Map<String, FieldDefinition> fieldDefs
) {
MethodDefinition ctor = classDef.declareConstructor(of(PUBLIC));
// super();
ctor.getBody().append(ctor.getThis()).invokeConstructor(InnerNode.class);
- // this._spec = new MyConfigurationSchema();
- ctor.getBody().append(ctor.getThis().setField(specField, newInstance(schemaClass)));
+ // this._spec# = new MyConfigurationSchema();
+ for (Map.Entry<Class<?>, FieldDefinition> e : specFields.entrySet())
+ ctor.getBody().append(ctor.getThis().setField(e.getValue(), newInstance(e.getKey())));
Review comment:
It looks like optimization, I propose to do it in a separate ticket.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] tkalkirill commented on a change in pull request #290: IGNITE-15047 Internal configuration
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on a change in pull request #290:
URL: https://github.com/apache/ignite-3/pull/290#discussion_r698570089
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java
##########
@@ -99,25 +124,24 @@ public ConfigurationRegistry(
return cgen.instantiateNode(rootKey.schemaClass());
}
};
- }
- /** {@inheritDoc} */
- @Override public void start() {
rootKeys.forEach(rootKey -> {
- cgen.compileRootSchema(rootKey.schemaClass());
+ cgen.compileRootSchema(rootKey.schemaClass(), extensions);
DynamicConfiguration<?, ?> cfg = cgen.instantiateCfg(rootKey, changer);
configs.put(rootKey.key(), cfg);
});
+ }
+ /** {@inheritDoc} */
+ @Override public void start() {
changer.start();
}
/** {@inheritDoc} */
@Override public void stop() {
- if (changer != null)
- changer.stop();
+ changer.stop();
Review comment:
Since the **changer** has become `final`.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org