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