[GitHub] [ignite-3] rpuch commented on a change in pull request #694: IGNITE-16545 Subscription to revision update in the test configuration framwork

[GitBox <gi...@apache.org>]

rpuch commented on a change in pull request #694:
URL: https://github.com/apache/ignite-3/pull/694#discussion_r816490268



##########
File path: modules/configuration/src/test/java/org/apache/ignite/internal/configuration/testframework/ConfigurationExtension.java
##########
@@ -102,28 +111,42 @@ public void beforeEach(ExtensionContext context) throws Exception {
 
         ExecutorService pool = context.getStore(NAMESPACE).get(POOL_KEY, ExecutorService.class);
 
-        for (Field field : getMatchingFields(testInstance.getClass())) {
+        StorageRevisionListenerHolderImpl revisionListenerHolder = new StorageRevisionListenerHolderImpl();
+
+        context.getStore(NAMESPACE).put(REVISION_LISTENER_HOLDER_KEY, revisionListenerHolder);
+
+        for (Field field : getInjectConfigurationFields(testInstance.getClass())) {
             field.setAccessible(true);
 
             InjectConfiguration annotation = field.getAnnotation(InjectConfiguration.class);
 
-            field.set(testInstance, cfgValue(field.getType(), annotation, cgen, pool));
+            field.set(testInstance, cfgValue(field.getType(), annotation, cgen, pool, revisionListenerHolder));
+        }
+
+        for (Field field : getInjectRevisionListenerHolderFields(testInstance.getClass())) {
+            field.setAccessible(true);
+
+            field.set(testInstance, revisionListenerHolder);
         }
     }
 
     /** {@inheritDoc} */
     @Override
     public void afterEach(ExtensionContext context) throws Exception {
         context.getStore(NAMESPACE).remove(CGEN_KEY);
+        context.getStore(NAMESPACE).remove(REVISION_LISTENER_HOLDER_KEY);
     }
 
     /** {@inheritDoc} */
     @Override
     public boolean supportsParameter(
-            ParameterContext parameterContext, ExtensionContext extensionContext
+            ParameterContext parameterContext,
+            ExtensionContext extensionContext
     ) throws ParameterResolutionException {
-        return parameterContext.isAnnotated(InjectConfiguration.class)
-                && supportType(parameterContext.getParameter().getType());
+        Class<?> parametrType = parameterContext.getParameter().getType();

Review comment:
       Looks like a typo: should be `parameterType`, not `parametrType`

##########
File path: modules/configuration/src/test/java/org/apache/ignite/internal/configuration/testframework/ConfigurationExtension.java
##########
@@ -216,13 +246,20 @@ private static Object cfgValue(
                     ConfigurationUtil.dropNulls(copy);
 
                     if (superRootRef.compareAndSet(sr, copy)) {
-                        Collection<CompletableFuture<?>> futures = notifyListeners(
+                        long storageRevision = revisionListenerHolder.storageRev.incrementAndGet();

Review comment:
       I suggest to encapsulate these accesses to the atomics and invocations of `incrementAndGet()` on them to two methods like `nextStorageRevision()` and `nextNotificationNumber()`. It would make it easier for the holder class to control its state.

##########
File path: modules/configuration/src/test/java/org/apache/ignite/internal/configuration/testframework/ConfigurationExtension.java
##########
@@ -255,27 +292,79 @@ public long notificationCount() {
         return cfgRef.get();
     }
 
-    /**
-     * Looks for the annotated field inside the given test class.
-     *
-     * @return Annotated fields.
-     */
-    private static List<Field> getMatchingFields(Class<?> testClass) {
+    private static List<Field> getInjectConfigurationFields(Class<?> testClass) {
         return AnnotationSupport.findAnnotatedFields(
                 testClass,
                 InjectConfiguration.class,
-                field -> supportType(field.getType()),
+                field -> isNameEndsWithConfiguration(field.getType()),
                 HierarchyTraversalMode.TOP_DOWN
         );
     }
 
+    private static List<Field> getInjectRevisionListenerHolderFields(Class<?> testClass) {
+        return AnnotationSupport.findAnnotatedFields(
+                testClass,
+                InjectRevisionListenerHolder.class,
+                field -> isRevisionListenerHolder(field.getType()),
+                HierarchyTraversalMode.TOP_DOWN
+        );
+    }
+
+    private static boolean isNameEndsWithConfiguration(Class<?> type) {

Review comment:
       `doesNameEndWithConfiguration()` seems to be more gramatically correct. Another option is `isNameEndingWithConfiguration()`.

##########
File path: modules/configuration/src/test/java/org/apache/ignite/internal/configuration/testframework/ConfigurationExtension.java
##########
@@ -255,27 +292,79 @@ public long notificationCount() {
         return cfgRef.get();
     }
 
-    /**
-     * Looks for the annotated field inside the given test class.
-     *
-     * @return Annotated fields.
-     */
-    private static List<Field> getMatchingFields(Class<?> testClass) {
+    private static List<Field> getInjectConfigurationFields(Class<?> testClass) {
         return AnnotationSupport.findAnnotatedFields(
                 testClass,
                 InjectConfiguration.class,
-                field -> supportType(field.getType()),
+                field -> isNameEndsWithConfiguration(field.getType()),
                 HierarchyTraversalMode.TOP_DOWN
         );
     }
 
+    private static List<Field> getInjectRevisionListenerHolderFields(Class<?> testClass) {
+        return AnnotationSupport.findAnnotatedFields(
+                testClass,
+                InjectRevisionListenerHolder.class,
+                field -> isRevisionListenerHolder(field.getType()),
+                HierarchyTraversalMode.TOP_DOWN
+        );
+    }
+
+    private static boolean isNameEndsWithConfiguration(Class<?> type) {
+        return type.getCanonicalName().endsWith("Configuration");
+    }
+
+    private static boolean isRevisionListenerHolder(Class<?> type) {
+        return ConfigurationStorageRevisionListenerHolder.class.isAssignableFrom(type);
+    }
+
     /**
-     * Checks that instance of the given class can be injected by the extension.
-     *
-     * @param type Field or parameter type.
-     * @return {@code true} if value of the given class can be injected.
+     * Implementation for {@link ConfigurationExtension}.
      */
-    private static boolean supportType(Class<?> type) {

Review comment:
       The old name (`supportType()`) was good in the sense that it was telling *what was the meaning* of the postfix check. The new name is accurate, but it does not tell you *why* it checks for the postfix.
   
   I would suggest to go back to the old approach (but change the name as this class now injects not only configurations, so my suggestion is to rename it to `supportsAsConfigurationType()`).




-- 
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