IGNITE-14180 Configuration notifications. #76

ibessonov · 2021-03-30T07:34:25Z

Signed-off-by: ibessonov bessonov.ip@gmail.com

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

# Conflicts: # modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

sashapolo · 2021-03-31T11:57:49Z

...on-processor/src/main/java/org/apache/ignite/configuration/processor/internal/Processor.java

        if (valueAnnotation != null) {
-            ClassName dynPropClass = ClassName.get(DynamicProperty.class);
-            ClassName confValueClass = ClassName.get(ConfigurationValue.class);
+            ClassName dynPropClass = ClassName.get("org.apache.ignite.configuration.internal", "DynamicProperty");


Just being curious: why did you change this?

Because I kept getting "NoClassDefFoundError" from that damn ITProcessorTest and this turned out to be the only way to fix it.

I think it deserves a comment

accidentallyget -- typo

sashapolo · 2021-03-31T12:03:30Z

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationRegistry.java

+            }
+        });
+
+        return CompletableFuture.allOf(futures.stream().map(listenerFut -> listenerFut.handle((res, throwable) -> {


Please extract the inner stream into a variable, it's hard to read otherwise

sashapolo · 2021-03-31T12:13:48Z

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

    /** Annotation classes mapped to validator objects. */
    private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();

+    /** */


This should be a proper javadoc)

sashapolo · 2021-03-31T12:14:09Z

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

+    @FunctionalInterface
+    public interface Notificator {
+        /** */
+        @NotNull CompletableFuture<?> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);


why not CompletableFuture<Void>?

Honestly, I see no difference between these two. Should I change it?

I think that the difference is that CompletableFuture<Void> explicitly states that there's no value inside it, while CompletableFuture<?> is closer to CompletableFuture<Object>

Ok, that makes sense. That's my laziness.

sashapolo · 2021-03-31T12:17:08Z

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

-    /** Constructor. */
-    public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+    /**
+     * @param notificator Closure to execute when update forom storage is received.


Suggested change

* @param notificator Closure to execute when update forom storage is received.

* @param notificator Closure to execute when update from the storage is received.

sashapolo · 2021-03-31T12:49:34Z

.../main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java

+        InnerNode newInnerNode,
+        DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+        long storageRevision,
+        List<CompletableFuture<?>> futures


Can we make it a return value?

You mean the list? It's more convenient to accumulate values this way I think, method is recursive and I wouldn't want to merge collections every time.

You can create a public method that will return a List, while inside you can call some recursive private methods that will populate this list. This is effectively the same approach, but I think it will make the API cleaner

So basically you mean that I should make this one private and create new public method that will return list, right? Ok, not a problem

We call this method from outside several times so I'll leave it with list parameter.

sashapolo · 2021-03-31T12:51:27Z

...ation/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationListener.java

+     * Called on property value update.
+     *
+     * @param ctx Notification context.
+     * @return Future that signifies end of listener execution. Can be {@code null}.


What does null mean in this context?

null is equivalent to completed future. I'll clarify it in the comment.

Can we return a completed future instead?

That's for convenience, people will implement this method and return completed future in most cases, why not allow them to return null instead?

Writing Future.completedFuture is not that hard, while dealing with both nulls and normal futures looks confusing and not a very clean design. Of course, that's mostly subjective.

Ok, I fixed it

sashapolo · 2021-03-31T12:53:19Z

...onfiguration/src/main/java/org/apache/ignite/configuration/storage/ConfigurationStorage.java

    void removeListener(ConfigurationStorageListener listener);
+
+    /**
+     * Notify storage that this specific revision was successfully handled and there's no necessity to repeat the same


Suggested change

* Notify storage that this specific revision was successfully handled and there's no necessity to repeat the same

* Notify storage that this specific revision was successfully handled and it is not necessary to repeat the same

sashapolo · 2021-03-31T12:56:44Z

...iguration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationUtil.java

    }

+    /**
+     * Nullifies values in {@code newNode} that 100% match with corresponding values in {@code curNode}.


Sorry, I don't understand what you are trying to say here...

Ok, I will try my best to rephrase it))

sashapolo · 2021-03-31T13:03:38Z

.../main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java

+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {


why do you need this class? It's only used in ConfigurationRegistry

Because methods in it are big and I don't want to have too much extra stuff in ConfigurationRegistry

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

alievmirza · 2021-04-01T14:00:54Z

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

    private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();

+    /**
+     * Closure interface to be used by configuration changer. Instance of this closur is passed into constructor and


Suggested change

* Closure interface to be used by configuration changer. Instance of this closur is passed into constructor and

* Closure interface to be used by configuration changer. An instance of this closure is passed into constructor and

Ok, thank you!

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

sashapolo · 2021-04-02T10:05:46Z

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java


        try {
-            change(defaultsNode, storageType).get();
+            change0(defaultsNode, storageInstances.get(storageType)).get();


It's interesting that you've decided to remove the method with the name change and not change0 =)

I'll rename it in the future)

why not now?

modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

...iguration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationUtil.java

…ation/ConfigurationChanger.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

…ation/internal/util/ConfigurationUtil.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

…ormat (#76)

ibessonov added 5 commits March 29, 2021 17:14

IGNITE-14180 Configuration notifications.

8d2bd06

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

Merge remote-tracking branch 'apache-gitbox/main' into ignite-14180

3c1db04

# Conflicts: # modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java

IGNITE-14180 Updates batch.

b70e3f9

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

IGNITE-14180 <checkstyle>

b96ba98

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

IGNITE-14180 Few javadocs.

0cd76f2

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

sashapolo reviewed Mar 31, 2021

View reviewed changes

IGNITE-14180 review fixes

50b898c

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

alievmirza reviewed Apr 1, 2021

View reviewed changes

ibessonov added 4 commits April 1, 2021 18:59

IGNITE-14180 review fixes

fb2cd3f

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

Merge remote-tracking branch 'apache-gitbox/main' into ignite-14180

2ddf5e8

IGNITE-14180 more review fixes

391ebc5

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

IGNITE-14180 typo

613c561

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

sashapolo reviewed Apr 2, 2021

View reviewed changes

ibessonov and others added 8 commits April 2, 2021 13:30

Update modules/configuration/src/main/java/org/apache/ignite/configur…

0cd5a69

…ation/ConfigurationChanger.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

Update modules/configuration/src/main/java/org/apache/ignite/configur…

85566e5

…ation/ConfigurationChanger.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

Update modules/configuration/src/main/java/org/apache/ignite/configur…

51cc74f

…ation/ConfigurationChanger.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

Update modules/configuration/src/main/java/org/apache/ignite/configur…

5347353

…ation/ConfigurationChanger.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

Update modules/configuration/src/main/java/org/apache/ignite/configur…

d763c58

…ation/internal/util/ConfigurationUtil.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

Update modules/configuration/src/main/java/org/apache/ignite/configur…

3c1d2b5

…ation/internal/util/ConfigurationUtil.java Co-authored-by: Alexander Polovtcev <alex.polovtcev@gmail.com>

IGNITE-14180 last fixes

8142054

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

IGNITE-14180 method renamed

0bfd57b

Signed-off-by: ibessonov <bessonov.ip@gmail.com>

sashapolo approved these changes Apr 2, 2021

View reviewed changes

ibessonov merged commit 297468b into apache:main Apr 2, 2021

ibessonov deleted the ignite-14180 branch June 4, 2021 07:22

ygerzhedovich pushed a commit that referenced this pull request Feb 23, 2024

GG-34789 Switch from previous version of IEP-54 to new binary tuple f…

2f801ba

…ormat (#76)

	* @param notificator Closure to execute when update forom storage is received.
	* @param notificator Closure to execute when update from the storage is received.

	* Notify storage that this specific revision was successfully handled and there's no necessity to repeat the same
	* Notify storage that this specific revision was successfully handled and it is not necessary to repeat the same

	* Closure interface to be used by configuration changer. Instance of this closur is passed into constructor and
	* Closure interface to be used by configuration changer. An instance of this closure is passed into constructor and

IGNITE-14180 Configuration notifications. #76

IGNITE-14180 Configuration notifications. #76

Uh oh!

Conversation

ibessonov commented Mar 30, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibessonov Apr 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibessonov Apr 1, 2021 •

edited

Loading