[pulsar-broker] PIP-100 Support pluggable topic factory #12235

rdhabalia · 2021-09-29T05:47:04Z

Motivation

Users select the topic type based on the application use case and requirement of topic behavior. However, in some circumstances, users need some additional behavior on top of the existing implementation and even would like to inject custom workflow in existing topic behavior. Such special circumstances are mostly needed when users would like to do smooth migrations of topics or pulsar clusters without impacting producer and consumer applications. In such scenarios, users can override publish or dispatch behavior of the topic and plug in the additional workflow. For example: perform dual write on multiple topics while migration or, skip messages published from the specific source without explicit publish failures, ignore specific subscription source without generating a client-side error, or without impacting client applications. This feature will be useful for any kind of migration where the pulsar cluster is serving live topics and require custom topic level treatment for flawless server-side migration and without impacting client application especially legacy applications which are hard to change.

Modification

Add broker side config topicFactoryClassName and TopicFactory interface.
Mark feature is an experimental feature to accommodate future API changes.

Result

Users can create topic with custom workflow for their server-side migration usecases.

Anonymitaet · 2021-09-30T07:35:04Z

Thanks for your contribution. For this PR, do we need to update docs?

(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

rdhabalia · 2021-11-20T05:42:41Z

ping

eolivelli

I left one comment on the API, can you please take a look ?

Generally I am not a fan of this kind of features because we allow users to interact with the internals of Pulsar and tweak the behaviour using the internal APIs that are not expected to be "maintained", I mean, that we can change from version to version (even point releases).

We are already full of this injection points, so I am not going to block this.
My "request changes" is about my comment to add a "close" method to the API

eolivelli · 2021-11-20T12:39:36Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/TopicFactory.java

+ */
+public interface TopicFactory {
+
+    <T extends Topic> T create(String topic, ManagedLedger ledger, BrokerService brokerService, Class<T> topicClazz);


should we add a close method ?
this will allow cleanup operations

@eolivelli addressed it.

extended Closeable interface

rdhabalia · 2021-12-21T22:36:45Z

@eolivelli addressed the comment. can you PTAL and unblock the PR.

rdhabalia · 2022-01-08T01:36:26Z

ping. @eolivelli can you please unblock the PR if your concern is addressed.

eolivelli

Hello @rdhabalia
Sorry for late reply
I read again the patch and I found that the new close method is not called, and I also added a couple of important comments in my opinion.

eolivelli · 2022-01-09T19:51:54Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/PersistentTopicE2ETest.java

+
+        @Override
+        public void close() throws IOException {
+            // No-op


Please verify the number of calls to this method

eolivelli · 2022-01-09T19:52:54Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

+                    return (T) newTopic;
+                }
+            } catch (Throwable e) {
+                log.warn("Failed to create persistent topic using factory {}, {}", topic, e.getMessage());


Here we should fail otherwise the system will be using the default implementation, leading to unpredictable behaviour

actually handling the exception and falling back to default implementation strategy was intentional to avoid unavailability of topic. however, I have updated it to throw exception for now so, we can merge this PR as it's open for almost 6 months now.

eolivelli · 2022-01-09T19:53:49Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

@@ -327,6 +328,7 @@ public BrokerService(PulsarService pulsar, EventLoopGroup eventLoopGroup) throws
                .newSingleThreadScheduledExecutor(new DefaultThreadFactory("pulsar-backlog-quota-checker"));
        this.authenticationService = new AuthenticationService(pulsar.getConfiguration());
        this.blockedDispatchers = new ConcurrentOpenHashSet<>();
+        this.topicFactory = createPersistentTopicFactory();


We are not shutting down (calling 'close') the new instance

eolivelli · 2022-01-09T19:54:26Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java

+            try {
+                return (TopicFactory) Class.forName(topicFactoryClassName).newInstance();
+            } catch (Exception e) {
+                log.warn("Failed to initialize topic factory class {}", topicFactoryClassName, e);


We should fail, otherwise the system will use the default configuration, leading to unexpected behaviour

rdhabalia · 2022-02-09T20:02:54Z

@eolivelli addressed the comments. please unblock when get a chance.

rdhabalia · 2022-02-11T18:11:50Z

@eolivelli ping

rdhabalia · 2022-03-07T22:31:28Z

ping

add Closeable fix test close factory and handle exception

momo-jun · 2022-08-19T02:03:10Z

Hi @rdhabalia, just wanted to check in on the docs because it didn't attach any doc label. Does it require to add or update any docs to help users better understand it? Thanks.

momo-jun · 2023-01-18T09:05:13Z

Since the content gap still exists, I attached the doc-required label for further tracking and fixing.
//cc @Technoboy-

chiragbparikh · 2023-05-04T22:18:24Z

@momo-jun and @rdhabalia Can we please get some documentation and clarification on how this feature can be used for Migration of topics and migration of topics across clusters?

momo-jun · 2023-05-30T09:13:13Z

@chiragbparikh, thanks for asking.
@rdhabalia, it would be great if you could provide docs to help users better understand this feature. If a feature is not documented, it doesn't exist :)

rdhabalia added the area/broker label Sep 29, 2021

rdhabalia added this to the 2.9.0 milestone Sep 29, 2021

rdhabalia requested review from merlimat, sijie, jiazhai, eolivelli and codelipenghui September 29, 2021 05:47

rdhabalia self-assigned this Sep 29, 2021

rdhabalia force-pushed the topic_factory branch from e3d3d2e to 649af8f Compare September 29, 2021 05:49

eolivelli modified the milestones: 2.9.0, 2.10.0 Oct 6, 2021

eolivelli requested changes Nov 20, 2021

View reviewed changes

rdhabalia force-pushed the topic_factory branch 2 times, most recently from 566d2ad to 46c5ecf Compare December 17, 2021 03:50

eolivelli requested changes Jan 9, 2022

View reviewed changes

codelipenghui modified the milestones: 2.10.0, 2.11.0 Jan 18, 2022

rdhabalia force-pushed the topic_factory branch from c905b37 to 7432bfb Compare February 9, 2022 18:31

rdhabalia mentioned this pull request Jun 9, 2022

[feat] [broker] support broker isolation for uniform load shedder strategy #13080

Open

eolivelli approved these changes Jul 20, 2022

View reviewed changes

rdhabalia force-pushed the topic_factory branch from f3ce1ed to de60f40 Compare July 20, 2022 06:38

[pulsar-broker] PIP-100 Support pluggable topic factory

31bb049

add Closeable fix test close factory and handle exception

rdhabalia force-pushed the topic_factory branch from de60f40 to 31bb049 Compare July 20, 2022 16:31

rdhabalia merged commit 6f38c5a into apache:master Jul 20, 2022

momo-jun added the doc-required Your PR changes impact docs and you will update later. label Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pulsar-broker] PIP-100 Support pluggable topic factory #12235

[pulsar-broker] PIP-100 Support pluggable topic factory #12235

rdhabalia commented Sep 29, 2021

Anonymitaet commented Sep 30, 2021

rdhabalia commented Nov 20, 2021

eolivelli left a comment

eolivelli Nov 20, 2021

rdhabalia Dec 20, 2021

rdhabalia Dec 21, 2021

rdhabalia commented Dec 21, 2021

rdhabalia commented Jan 8, 2022

eolivelli left a comment

eolivelli Jan 9, 2022

eolivelli Jan 9, 2022

rdhabalia Feb 9, 2022

eolivelli Jan 9, 2022

eolivelli Jan 9, 2022

rdhabalia commented Feb 9, 2022

rdhabalia commented Feb 11, 2022

rdhabalia commented Mar 7, 2022

momo-jun commented Aug 19, 2022

momo-jun commented Jan 18, 2023

chiragbparikh commented May 4, 2023

momo-jun commented May 30, 2023

[pulsar-broker] PIP-100 Support pluggable topic factory #12235

[pulsar-broker] PIP-100 Support pluggable topic factory #12235

Conversation

rdhabalia commented Sep 29, 2021

Motivation

Modification

Result

Anonymitaet commented Sep 30, 2021

rdhabalia commented Nov 20, 2021

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdhabalia commented Dec 21, 2021

rdhabalia commented Jan 8, 2022

eolivelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdhabalia commented Feb 9, 2022

rdhabalia commented Feb 11, 2022

rdhabalia commented Mar 7, 2022

momo-jun commented Aug 19, 2022

momo-jun commented Jan 18, 2023

chiragbparikh commented May 4, 2023

momo-jun commented May 30, 2023