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