tkalkirill commented on code in PR #2386:
URL: https://github.com/apache/ignite-3/pull/2386#discussion_r1285527587
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/annotation/ConfigurationExtension.java:
##########
@@ -39,5 +40,12 @@
@Target(TYPE)
@Retention(RUNTIME)
@Documented
-public @interface InternalConfiguration {
+public @interface ConfigurationExtension {
+
Review Comment:
```suggestion
```
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/annotation/ConfigurationExtension.java:
##########
@@ -39,5 +40,12 @@
@Target(TYPE)
@Retention(RUNTIME)
@Documented
-public @interface InternalConfiguration {
+public @interface ConfigurationExtension {
+
+ /**
+ * Controls whether this configuration is part of the public configuration
or is hidden from the end user.
+ * An extension is internal by default.
+ */
+ boolean internal() default true;
Review Comment:
I suggest the default **FALSE**.
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/RootKey.java:
##########
@@ -38,8 +38,8 @@ public class RootKey<T extends ConfigurationTree<VIEWT, ?>,
VIEWT> {
/** Schema class for the root. */
private final Class<?> schemaClass;
- /** Marked with {@link InternalConfiguration}. */
- private final boolean internal;
+ /** Marked with {@link ConfigurationExtension} and internal = true. */
+ private final boolean privateExtension;
Review Comment:
Let it be the previous name.
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -78,12 +78,12 @@ public interface ConfigurationModule {
}
/**
- * Returns classes of internal schema extensions (annotated with {@link
InternalConfiguration})
+ * Returns classes of schema extensions (annotated with {@link
ConfigurationExtension})
* provided by this module.
*
Review Comment:
```suggestion
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/RootInnerNode.java:
##########
@@ -28,8 +28,8 @@ public class RootInnerNode {
/** Root node. */
private final InnerNode node;
- /** Internal configuration. */
- private final boolean internal;
+ /** Internal configuration extensions. */
+ private final boolean privateExtension;
Review Comment:
It is worth reverting to the original name.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/RootInnerNode.java:
##########
@@ -61,11 +61,12 @@ public InnerNode node() {
}
/**
- * Check if the root configuration is marked with {@link
InternalConfiguration}.
+ * Check if the root configuration is marked with {@link
ConfigurationExtension} an is internal.
*
* @return {@code true} if the root configuration is internal.
*/
- public boolean internal() {
- return internal;
+ public boolean privateExtension() {
+ return privateExtension;
}
+
Review Comment:
```suggestion
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java:
##########
@@ -209,12 +210,21 @@ public synchronized void compileRootSchema(
? concat(schemaFields(schemaClass),
schemaFields(schemaSuperClass))
: schemaFields(schemaClass);
- Collection<Field> internalExtensionsFields =
extensionsFields(internalExtensions, true);
+ Set<Class<?>> privateExtensions = extensions.stream()
+ .filter(ConfigurationUtil::isPrivateExtension)
+ .collect(toSet());
+ Set<Class<?>> publicExtensions = extensions.stream()
Review Comment:
```suggestion
Set<Class<?>> publicExtensions = extensions.stream()
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java:
##########
@@ -264,10 +264,10 @@ public void setInjectedNameFieldValue(String value) {
}
/**
- * Returns schemas for {@link InternalConfiguration internal configuration
extensions}.
+ * Returns schemas for {@link ConfigurationExtension configuration
extensions}.
*/
@Nullable
Review Comment:
```suggestion
```
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java:
##########
@@ -549,21 +549,21 @@ void testCheckConfigurationTypeNoError() {
@Test
void testInternalSchemaExtensions() {
Review Comment:
Please add test for public schema extensions
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -78,12 +78,12 @@ public interface ConfigurationModule {
}
/**
- * Returns classes of internal schema extensions (annotated with {@link
InternalConfiguration})
+ * Returns classes of schema extensions (annotated with {@link
ConfigurationExtension})
* provided by this module.
*
- * @return internal schema extensions' classes
+ * @return schema extensions' classes
Review Comment:
```suggestion
```
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -641,7 +641,7 @@ private void validateConfigurationSchemaClass(TypeElement
clazz, List<VariableEl
String.format("%s must end with '%s'",
clazz.getQualifiedName(), CONFIGURATION_SCHEMA_POSTFIX));
}
- if (clazz.getAnnotation(InternalConfiguration.class) != null) {
+ if (clazz.getAnnotation(ConfigurationExtension.class) != null) {
validateInternalConfiguration(clazz, fields);
Review Comment:
I think the method should be renamed.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/RootInnerNode.java:
##########
@@ -61,11 +61,12 @@ public InnerNode node() {
}
/**
- * Check if the root configuration is marked with {@link
InternalConfiguration}.
+ * Check if the root configuration is marked with {@link
ConfigurationExtension} an is internal.
*
* @return {@code true} if the root configuration is internal.
*/
- public boolean internal() {
- return internal;
+ public boolean privateExtension() {
+ return privateExtension;
Review Comment:
It is worth reverting to the original name.
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -244,7 +244,7 @@ private boolean process0(RoundEnvironment roundEnvironment)
{
boolean isRootConfig =
clazz.getAnnotation(ConfigurationRoot.class) != null;
// Is the internal configuration.
- boolean isInternalConfig =
clazz.getAnnotation(InternalConfiguration.class) != null;
+ boolean isInternalConfig =
clazz.getAnnotation(ConfigurationExtension.class) != null;
Review Comment:
The check is not complete, the check of the flag is missing, or the field
needs to be renamed.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/DynamicConfiguration.java:
##########
@@ -184,13 +184,13 @@ public Map<String, ConfigurationProperty<?>>
touchMembers() {
public abstract Class<?> configType();
/**
- * Returns the interfaces of the {@link InternalConfiguration internal
configuration extensions} for example
+ * Returns the interfaces of the {@link ConfigurationExtension
configuration extensions} for example
* {@code InternalTableConfiguration}, {@code null} if absent.
*
* @throws UnsupportedOperationException In the case of a named list.
*/
@Nullable
Review Comment:
```suggestion
```
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/RootKey.java:
##########
@@ -87,12 +88,12 @@ public Class<?> schemaClass() {
}
/**
- * Check if the root configuration is marked with {@link
InternalConfiguration}.
+ * Check if the root configuration is marked with {@link
ConfigurationExtension}.
*
* @return {@code true} if the root configuration is internal.
*/
- public boolean internal() {
- return internal;
+ public boolean privateExtension() {
Review Comment:
Let it be the previous name.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/NamedListConfiguration.java:
##########
@@ -196,7 +196,7 @@ public Class<?> configType() {
/** {@inheritDoc} */
@Override
- public @Nullable Class<?>[] internalConfigTypes() {
+ public @Nullable Class<?>[] extensionConfigTypes() {
Review Comment:
```suggestion
public Class<?> @Nullable [] extensionConfigTypes() {
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationImplAsmGenerator.java:
##########
@@ -150,20 +150,22 @@ class ConfigurationImplAsmGenerator extends
AbstractAsmGenerator {
ConfigurationImplAsmGenerator(
ConfigurationAsmGenerator cgen,
Class<?> schemaClass,
- Set<Class<?>> internalExtensions,
+ Set<Class<?>> extensions,
Set<Class<?>> polymorphicExtensions,
List<Field> schemaFields,
- Collection<Field> internalFields,
+ Collection<Field> privateExtensionFields,
Review Comment:
internal
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -232,20 +232,22 @@ class InnerNodeAsmGenerator extends AbstractAsmGenerator {
InnerNodeAsmGenerator(
ConfigurationAsmGenerator cgen,
Class<?> schemaClass,
- Set<Class<?>> internalExtensions,
+ Set<Class<?>> extensions,
Set<Class<?>> polymorphicExtensions,
List<Field> schemaFields,
- Collection<Field> internalFields,
+ Collection<Field> privateExtensionFields,
Review Comment:
internal
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/DirectProxyAsmGenerator.java:
##########
@@ -100,18 +100,20 @@ class DirectProxyAsmGenerator extends
AbstractAsmGenerator {
DirectProxyAsmGenerator(
ConfigurationAsmGenerator cgen,
Class<?> schemaClass,
- Set<Class<?>> internalExtensions,
+ Set<Class<?>> extensions,
List<Field> schemaFields,
- Collection<Field> internalExtensionsFields,
+ Collection<Field> privateExtensionFields,
Review Comment:
internal
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/DynamicConfiguration.java:
##########
@@ -184,13 +184,13 @@ public Map<String, ConfigurationProperty<?>>
touchMembers() {
public abstract Class<?> configType();
/**
- * Returns the interfaces of the {@link InternalConfiguration internal
configuration extensions} for example
+ * Returns the interfaces of the {@link ConfigurationExtension
configuration extensions} for example
* {@code InternalTableConfiguration}, {@code null} if absent.
*
* @throws UnsupportedOperationException In the case of a named list.
*/
@Nullable
- public Class<?>[] internalConfigTypes() {
+ public Class<?>[] extensionConfigTypes() {
Review Comment:
```suggestion
public Class<?> @Nullale [] extensionConfigTypes() {
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/ConfigurationAsmGenerator.java:
##########
@@ -209,12 +210,21 @@ public synchronized void compileRootSchema(
? concat(schemaFields(schemaClass),
schemaFields(schemaSuperClass))
: schemaFields(schemaClass);
- Collection<Field> internalExtensionsFields =
extensionsFields(internalExtensions, true);
+ Set<Class<?>> privateExtensions = extensions.stream()
Review Comment:
internal
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java:
##########
@@ -677,6 +677,16 @@ public static Collection<Field>
extensionsFields(Collection<Class<?>> extensions
}
}
+ public static boolean isPrivateExtension(Class<?> schemaClass) {
Review Comment:
internal
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/AbstractAsmGenerator.java:
##########
@@ -64,17 +64,20 @@ abstract class AbstractAsmGenerator {
/** Configuration schema class. */
final Class<?> schemaClass;
- /** Internal extensions of the configuration schema. */
- final Set<Class<?>> internalExtensions;
+ /** Extensions of the configuration schema (both private and public). */
+ final Set<Class<?>> extensions;
/** Polymorphic extensions of the configuration schema. */
final Set<Class<?>> polymorphicExtensions;
/** Fields of the schema class. */
final List<Field> schemaFields;
- /** Fields of internal extensions of the configuration schema. */
- final Collection<Field> internalFields;
+ /** Fields of private extensions of the configuration schema. */
+ final Collection<Field> privateExtensionFields;
Review Comment:
internal
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java:
##########
@@ -264,10 +264,10 @@ public void setInjectedNameFieldValue(String value) {
}
/**
- * Returns schemas for {@link InternalConfiguration internal configuration
extensions}.
+ * Returns schemas for {@link ConfigurationExtension configuration
extensions}.
*/
@Nullable
- public Class<?>[] internalSchemaTypes() {
+ public Class<?>[] extensionSchemaTypes() {
Review Comment:
```suggestion
public Class<?> @Nullable [] extensionSchemaTypes() {
```
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
