CURATOR-718: Refactor CuratorFramework inheritance hierarchy by composing functionalities #517

kezhuw · 2024-12-31T05:13:41Z

This closes #1233.

Currently, the CuratorFrameworkImpl hierarchy is neither simple nor composable.

Ideally, there should be only one instance of CuratorFrameworkImpl. Additional functionalities should be added by intercepting methods on purpose, but not by cloning through CuratorFrameworkImpl(CuratorFrameworkImpl parent).

We could take CURATOR-626 and CURATOR-710 as lessons to know how brittle the currently hierarchy.

kezhuw · 2024-12-31T05:24:28Z

I leave CURATOR-710 to #508.

To resolve CURATOR-710, I think we need to decide whether CuratorFramework::usingNamespace should inherit functionalities of this ? If it is yes, then we probably could fix CURATOR-710 in a straightfoward way (comparing to trick fix in #508).

kezhuw

This is a huge pr. But most changes are migrating references from CuratorFrameworkImpl to InternalCuratorFramework so we can customize functionalities by overriding methods in InternalCuratorFramework but not CuratorFrameworkImpl. Now, there will be only one CuratorFrameworkImpl per curator client.

kezhuw · 2024-12-31T05:32:38Z

curator-framework/src/main/java/org/apache/curator/framework/imps/InternalCuratorFramework.java

+ *
+ * <p>An instance of {@link CuratorFramework} MUST BE an instance of {@link InternalCuratorFramework}.
+ */
+public abstract class InternalCuratorFramework implements CuratorFramework {


This is the most notable change.

NamespaceFacade and WatcherRemovalFacade are now subclasses of DelegatingCuratorFramework and InternalCuratorFramework but not CuratorFrameworkImpl.

kezhuw · 2024-12-31T05:51:09Z

...cipes/src/test/java/org/apache/curator/framework/recipes/leader/TestLeaderSelectorEdges.java

@@ -98,15 +97,13 @@ public void flappingTest() throws Exception {
        } finally {
            try {
                leaderSelector1.close();
-            } catch (IllegalStateException e) {
-                fail(e.getMessage());


This shadow the real cause.

See https://ge.apache.org/s/rcubepxch5ibu/tests/goal/org.apache.curator:curator-recipes:surefire:test@default-test/details/org.apache.curator.framework.recipes.leader.TestLeaderSelectorEdges/flappingTest?top-execution=1

tisonkun · 2025-01-16T16:18:40Z

@kezhuw would you consider this is a release blocker? I may expect to call a release by the end of Feb. and now try to collaborate to converge the ongoing PRs.

kezhuw · 2025-01-16T16:29:35Z

would you consider this is a release blocker?

No. It is not even a bug.

…sing functionalities Currently, the `CuratorFrameworkImpl` hierarchy is neither simple nor composable. Ideally, there should be only one instance of `CuratorFrameworkImpl`. Additional functionalities should be added by intercepting methods on purpose, but not by cloning through `CuratorFrameworkImpl(CuratorFrameworkImpl parent)`. We could take CURATOR-626 and CURATOR-710 as lessons to know how brittle the currently hierarchy.

tisonkun · 2025-01-25T03:37:16Z

...or-framework/src/main/java/org/apache/curator/framework/imps/DelegatingCuratorFramework.java

+    }
+
+    @Override
+    public <DATA_TYPE> void processBackgroundOperation(


Suggested change

public <DATA_TYPE> void processBackgroundOperation(

<DATA_TYPE> void processBackgroundOperation(

.. and may need a re-format

tisonkun · 2025-01-25T03:38:22Z

curator-framework/src/main/java/org/apache/curator/framework/imps/InternalCuratorFramework.java

+ *
+ * <p>An instance of {@link CuratorFramework} MUST BE an instance of {@link InternalCuratorFramework}.
+ */
+public abstract class InternalCuratorFramework implements CuratorFramework {


Personally, I'd prefer to name it CuratorFrameworkBase since it's used outside (as method parameter) rather than internal.

Renamed.

By internal, I mean it should not be used outside of curator. Currently, only impls/details packages use it. Add it is public for private usages for caution.

tisonkun · 2025-01-25T03:39:29Z

...or-framework/src/main/java/org/apache/curator/framework/imps/DelegatingCuratorFramework.java

+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.server.quorum.flexible.QuorumVerifier;
+
+public class DelegatingCuratorFramework extends InternalCuratorFramework {


Add some class comment for its purpose?

Also this seems another abstract class rather than an instantiable one. And we don't need impl start and close in such a bias.

DelegatingCuratorFramework is now abstract and start/close has been moved to subclasses.

tisonkun · 2025-01-25T03:43:03Z

...or-framework/src/main/java/org/apache/curator/framework/imps/DelegatingCuratorFramework.java

+    @Override
+    public void start() {
+        throw new UnsupportedOperationException();
+    }
+
+    @Override
+    public void close() {
+        throw new UnsupportedOperationException();
+    }


Or somewhat delegate to client.start/close also?

Currently, only CuratorFrameworkImpl::start/CuratorFrameworkImpl::close take effects, start/close in other facades throw. I think we should keep this behavior unchanged in an refactor.

Yeah I noticed that this is the original manner. Then it's fine. (Although I think it's by accident, cc @Randgalt do you remember the reason why we don't delegate here?

Hmm, I don't think NamespaceFacade::close should close curator.

tisonkun · 2025-01-25T03:44:50Z

...or-framework/src/main/java/org/apache/curator/framework/imps/DelegatingCuratorFramework.java

+import org.apache.zookeeper.Watcher;
+import org.apache.zookeeper.server.quorum.flexible.QuorumVerifier;
+
+public class DelegatingCuratorFramework extends InternalCuratorFramework {


Also can be package private.

tisonkun

Rest generally looks good to me.

…y composing functionalities Notable changes to last review: 1. Rename `InternalCuratorFramework` to `CuratorFrameworkBase`. 2. Make `DelegatingCuratorFramework` `abstract` and package `private`. 3. Move `start`/`close` from `DelegatingCuratorFramework` to subclasses.

kezhuw · 2025-02-07T05:28:46Z

@tisonkun Sorry for being so late! I have pushed a fixup commit to solve the review comments.

kezhuw force-pushed the CURATOR-718-refactor-CuratorFramework-inheritance-hierarchy branch from ff34371 to c2195e1 Compare December 31, 2024 05:19

kezhuw force-pushed the CURATOR-718-refactor-CuratorFramework-inheritance-hierarchy branch from c2195e1 to 3ecccc6 Compare December 31, 2024 05:25

kezhuw commented Dec 31, 2024

View reviewed changes

tisonkun self-requested a review January 14, 2025 13:26

tisonkun mentioned this pull request Jan 16, 2025

CURATOR-710: Fix leaking watch in EnsembleTracker #508

Merged

kezhuw force-pushed the CURATOR-718-refactor-CuratorFramework-inheritance-hierarchy branch 2 times, most recently from 288799c to 2cb13cf Compare January 20, 2025 02:50

kezhuw force-pushed the CURATOR-718-refactor-CuratorFramework-inheritance-hierarchy branch from 2cb13cf to c47cc6f Compare January 20, 2025 04:25

tisonkun reviewed Jan 25, 2025

View reviewed changes

tisonkun approved these changes Feb 7, 2025

View reviewed changes

tisonkun merged commit f0646f9 into apache:master Feb 17, 2025
10 checks passed

	public <DATA_TYPE> void processBackgroundOperation(
	<DATA_TYPE> void processBackgroundOperation(

CURATOR-718: Refactor CuratorFramework inheritance hierarchy by composing functionalities #517

CURATOR-718: Refactor CuratorFramework inheritance hierarchy by composing functionalities #517

Uh oh!

Conversation

kezhuw commented Dec 31, 2024 • edited by tisonkun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kezhuw commented Dec 31, 2024

Uh oh!

kezhuw left a comment

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!

tisonkun commented Jan 16, 2025

Uh oh!

kezhuw commented Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

tisonkun left a comment

Choose a reason for hiding this comment

Uh oh!

kezhuw commented Feb 7, 2025

Uh oh!

Uh oh!

Uh oh!

kezhuw commented Dec 31, 2024 •

edited by tisonkun

Loading

kezhuw commented Jan 16, 2025 •

edited

Loading