Adding configuration to disable auto-topic creation #3450

ConcurrencyPractitioner · 2019-01-27T01:09:19Z

Feature Request

There was a request for this feature in #3436.

Documentation

Will add to documentation a description of the new feature.

sijie

I think a better approach is to disable auto creation at broker side. So administrators can disable this behavior while creating the cluster.

ConcurrencyPractitioner · 2019-01-27T02:17:27Z

Ok, I found that the change should be in PersistentTopicsBase L946. I will work on it from there.

sijie · 2019-01-27T03:12:37Z

@ConcurrencyPractitioner : cool.

pulsar broker uses managed ledger to store the topic partitions. there is actually a config in managed ledger createIfMissing - https://github.com/apache/pulsar/blob/master/managed-ledger/src/main/java/org/apache/bookkeeper/mledger/ManagedLedgerConfig.java#L71 . if auto create topic is disabled, then you can disable createIfMissing.

Hope this gives you some entries for you to check out the code.

ConcurrencyPractitioner · 2019-01-27T19:17:55Z

Ok, I did some digging and here's what I found:
createIfMissing is used when seeing if a Z-Node is already built for a ledger. If a Z-Node does not exist, then a new one will be created (i.e. if createIfMissing equals true).

I found that PersistentTopicsBase will (probably?) create a new PersistentTopic if there is no partition metadata (partitionMetadata.partitions == 0 ) for that particular store. At this point, I'm not too sure what this means.

My best guess is that if there is no Z-Node for a ledger, then that means no partitions exists for it either. @sijie

At this point, I'm not too sure what connection createIfMissing has with partitionMetadata.partitions.

Edit: Ok, after some more digging. I have found that when getPartitionedTopicMetadata is called in PersistentTopicsBase, a CompletableFuture will be created. I found that if the value returned is null (i.e. if no corresponding Z-Node exists, rc == Code.NONODE.intValue()), then it indicates that a particular topic is not partitioned. So I think this basically means my inference is right.

ConcurrencyPractitioner · 2019-01-28T04:20:04Z

Ok, there are some kinks in my understanding. Wouldn't disabling createIfMissing also mean that no non-partitioned topics will be created? I'm not too sure actually.

sijie · 2019-01-28T20:29:36Z

@ConcurrencyPractitioner

Ok, there are some kinks in my understanding. Wouldn't disabling createIfMissing also mean that no non-partitioned topics will be created? I'm not too sure actually.

Ah, you are right.

So I think in the task, you should do following:

provide a flag allowTopicAutoCreation, defaults to true
the entrypoint for creating/loading topic is BrokerService#getOrCreateTopic.
getOrCreateTopic will use allowTopicAutoCreation as createIfMissing.
the only exception is to use createIfMissing=true when BrokerService#getOrCreateTopic is called in admin rest endpoint (PersistentTopicsBase.java)
you need to provide a rest endpoint in PersistentTopicsBase to create non-partitioned topic
you need to improve createPartitionedTopic in PersistentTopicsBase to create all the partitions, because rest admin endpoint is the only entrypoint to create partitions when allowTopicAutoCreation is set to false.

Hope this clarify the tasks to do for this feature.

sijie

I think allowAutoTopicCreation should be a flag in broker configuration, rather than in ManagedLedgerConfig. So BrokerService would set different createIfMissing values at different scenarios based on whether allowAutoTopicCreation is set to true or not.

Also we need add new unit tests and integration tests to cover this feature.

sijie · 2019-02-02T01:02:24Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

        });
    }
-
+    


nit: remove trailing spaces

ConcurrencyPractitioner · 2019-02-04T03:54:15Z

Hi @sijie

I've moved the config, but where would you suggest that we add the test? I've made some efforts and discovered a couple of candidate test locations like PulsarFunctionAdminTest and BrokerServiceTest. However, they don't seem to be testing configs related to Managed Ledger and the creation of z-nodes.

sijie · 2019-02-04T07:55:28Z

@ConcurrencyPractitioner

I think you need to verify following cases:

when autoCreateTopic is disabled, 1) you can't publish or consume from topics when topics are not explicitly created. 2) you can create partitioned and non-partitions topics from pulsar admin rest endpoint.

You can add a new test case to pulsar-broker module. There are plenty test cases using MockedPulsarServiceBaseTest, you can check them out as examples.

ConcurrencyPractitioner · 2019-02-06T05:20:25Z

HI @sijie,

I'm currently using Eclipse to develop for Pulsar. What IDE do you use for Pulsar?
On a side note, do you know how to get the .class file for a .java file like ServiceConfiguration and import it into an IDE? Without it, the Getter() and Setter() methods that are lombok generated is not recognized by the IDE (for me, Eclipse) as methods which exist (so they are considered illegal syntax).

sijie · 2019-02-06T05:30:44Z

@ConcurrencyPractitioner

I am using Intellij for my java development.

For Lombok with Eclipse, I think if you run java -jar /path/to/lombok.jar, it will start a lombok installation window, you can follow the instructions to configure your IDE.

Here is one blog post about that - https://howtodoinjava.com/automation/lombok-eclipse-installation-examples/ Hope that helps.

ConcurrencyPractitioner · 2019-02-06T06:06:57Z

Thanks for the pointers. Just wanting to confirm, is it the community version or the licensed version?

Also, I did some work on PersistentTopicsTest and attempted to add a new test.
Here's basically what it did:

    	final String nonPartitionTopic = "nonPartitionedTopic";
    	persistentTopics.createSubscription(testTenant, testNamespace, nonPartitionTopic, "test", true, (MessageIdImpl) MessageId.latest);
    	List<String> subscriptions =  persistentTopics.getSubscriptions(testTenant, testNamespace, nonPartitionTopic + "-partition-0", true);
        Assert.assertTrue(subscriptions.contains("test"));
        persistentTopics.deleteSubscription(testTenant, testNamespace, nonPartitionTopic, "test", true);
        subscriptions =  persistentTopics.getSubscriptions(testTenant, testNamespace, nonPartitionTopic + "-partition-0", true);
        Assert.assertTrue(subscriptions.isEmpty());

I also modified the config value i.e. pulsar.getConfiguration().setAllowAutoTopicCreation(false)

WDYT of this?

sijie · 2019-02-06T06:23:03Z

Just wanting to confirm, is it the community version or the licensed version?

you mean Intellij? I am using the community version.

ConcurrencyPractitioner · 2019-02-10T03:14:11Z

Retest this please.

ConcurrencyPractitioner · 2019-02-11T05:12:44Z

@sijie Ok, added the PersistentTopicsTest. There are a couple more tests to go, but other than that, this PR should be getting close.

sijie

@ConcurrencyPractitioner overall looks good to me.

I think there are two remaining tasks for this. if you can do them in a follow up pull request, that would be great.

currently we don't have any rest endpoint for creating non-partitioned topics. so if we disable auto-creation of topics, people doesn't have any mechanisms to create non-partitioned topics.

so we need to add a rest endpoint for creating non-partitioned topic. this includes both admin rest endpoint change, a commandline cli change and documentation. I am wondering if you are interested in continuing the contributions in a subsequent pull request.

sijie · 2019-02-11T05:21:21Z

managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/MetaStoreImplZookeeper.java

                        }
                    } else if (rc == Code.NONODE.intValue()) {
                        // Z-node doesn't exist
+                    	log.warn("createIfMissing has value of {}", createIfMissing);


nit: is this for debugging purpose?

sijie · 2019-02-11T05:23:05Z

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

+    	return getOrCreateTopic(topic, false /* use allowAutoTopicCreation */);
+    }
+
+    public CompletableFuture<Topic> getOrCreateTopic(String topic, boolean defaultConfig) {


nit: defaultConfig => useAllowAutoTopicCreationSetting

sijie · 2019-02-11T05:23:33Z

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

+
+    public CompletableFuture<Topic> getOrCreateTopic(String topic, boolean defaultConfig) {
+    	boolean createIfMissing = defaultConfig ? pulsar.getConfiguration().isAllowAutoTopicCreation() : true;
+    	log.warn("The createIfMissing value had been set to {}", createIfMissing);


hmm, can you move the logging to debug, otherwise it will be very annoying.

sijie · 2019-02-11T05:24:19Z

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

        }

        getManagedLedgerConfig(topicName).thenAccept(managedLedgerConfig -> {
+        	log.warn("createIfMissing has been set to {}", createIfMissing);


remove this comment or turn it to log.debug

@ConcurrencyPractitioner can you also remove this log.warn statement?

sijie · 2019-02-11T05:24:54Z

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


    public CompletableFuture<Optional<Topic>> getTopicIfExists(final String topic) {
-        return getTopic(topic, false /* createIfMissing */);
+        return getTopic(topic, false /* use allowAutoTopicCreation */);


should this be true?

Uh no. getTopicIfExists should never create a topic. That is the purpose of this method.

oh I see. so the comment of the flag here is wrong, no?

it should be /* createIfMissing */

Oh, good point. I goofed.

sijie · 2019-02-11T05:25:05Z

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


    public CompletableFuture<Topic> getOrCreateTopic(final String topic) {
-        return getTopic(topic, true /* createIfMissing */).thenApply(Optional::get);
+    	return getOrCreateTopic(topic, false /* use allowAutoTopicCreation */);


should this be true?

sijie · 2019-02-11T05:26:10Z

pulsar-broker/src/test/java/org/apache/pulsar/client/impl/BrokerClientIntegrationTest.java

 import org.testng.annotations.AfterMethod;
 import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.DataProvider;
+import org.testng.annotations.Ignore;


it is not needed any more no?

ConcurrencyPractitioner · 2019-02-12T05:35:28Z

Retest this please.

sijie · 2019-02-13T02:06:16Z

@ConcurrencyPractitioner I have a comment left regarding the comment. otherwise, this change is ready to go.

ConcurrencyPractitioner · 2019-02-13T05:13:21Z

Ok @sijie Done.

ivankelly · 2019-02-13T15:29:47Z

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

                            }
                        }

-                        service.getOrCreateTopic(topicName.toString())


you don't need true here, it defaults to true.

ivankelly · 2019-02-13T15:30:01Z

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

                        log.info("[{}][{}] Creating producer. producerId={}", remoteAddress, topicName, producerId);

-                        service.getOrCreateTopic(topicName.toString()).thenAccept((Topic topic) -> {
+                        service.getOrCreateTopic(topicName.toString(), true).thenAccept((Topic topic) -> {


ivankelly · 2019-02-13T15:42:35Z

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

+    	return getOrCreateTopic(topic, true /* use allowAutoTopicCreation */);
+    }
+
+    public CompletableFuture<Topic> getOrCreateTopic(String topic, boolean useAllowAutoTopicCreationSetting) {


Instead of creating a new override for getOrCreateTopic, make getTopic public.
Then the one place that calls getOrCreateTopic(..., false) can instead call getTopic(..., true), and getOrCreateTopic() can become

public CompletableFuture<Topic> getOrCreateTopic(final String topic) { return getTopic(topic, pulsar.getConfiguration().isAllowAutoTopicCreation()).thenApply(Optional::get); }

ConcurrencyPractitioner · 2019-02-14T03:20:27Z

About the rest endpoint @sijie I'm good on following through about creating it. Although to be fair, where would be the best place to add the command line client change?

ConcurrencyPractitioner · 2019-02-14T03:53:05Z

Retest this please.

sijie · 2019-02-14T07:33:08Z

@ConcurrencyPractitioner it should be in the same location as createPartitionedTopic (both broker and client sides)

ivankelly

Looks good to me. 👍

ConcurrencyPractitioner · 2019-02-15T03:36:07Z

@sijie Should I add the endpoint in a separate PR or this one?

sijie · 2019-02-15T04:54:07Z

@ConcurrencyPractitioner let's do it in a separated PR

sijie · 2019-02-15T04:54:20Z

run integration tests

ConcurrencyPractitioner · 2019-02-16T05:36:49Z

@sijie or @ivankelly Do you think this could be merged?

sijie · 2019-02-16T09:09:17Z

@ConcurrencyPractitioner well done

Master Issue: #4926 ### Motivation Curently the partitioned-topic and non-partitioned topic is a little confuse for users. in PR #3450 we add config for auto-topic-creation. We could leverage this config to provide some more config for auto-topic-creation. ### Modifications - Add `allowAutoTopicCreationType` and `allowAutoTopicCreationNumPartitions` to configuration. - Users can use both configurations when they decide to create a topic automatically. - Add test. - Update doc.

Master Issue: #4926 Curently the partitioned-topic and non-partitioned topic is a little confuse for users. in PR #3450 we add config for auto-topic-creation. We could leverage this config to provide some more config for auto-topic-creation. - Add `allowAutoTopicCreationType` and `allowAutoTopicCreationNumPartitions` to configuration. - Users can use both configurations when they decide to create a topic automatically. - Add test. - Update doc. (cherry picked from commit 547c421)

Adding configuration to disable auto-topic creation

f5e1350

sijie reviewed Jan 27, 2019

View reviewed changes

Adding config

81f5e8a

sijie assigned ConcurrencyPractitioner Jan 28, 2019

sijie added type/feature The PR added a new feature or issue requested a new feature area/broker labels Jan 28, 2019

ConcurrencyPractitioner added 7 commits January 28, 2019 19:51

Starting to add backbone

ab4325c

Adding work for completableFuture

01ff186

Fixing issue

80dd5f9

Adding detail for PersistentTopicBases

5dc6920

Simplifying call structure

d2eed96

Fixing compilation error

d0094a4

Fixing compilation errror

1c72664

sijie requested changes Feb 2, 2019

View reviewed changes

ConcurrencyPractitioner added 2 commits February 1, 2019 17:23

Modifying config location

36abfed

Fixing config error

8302804

ConcurrencyPractitioner added 3 commits February 5, 2019 20:22

Adding test for PersistentTopicsBAse

b493cf8

Fixing extra lines

52faeaa

Reversing unneccesary test

5aec8dd

Removing ignore annotation

c409248

Fixing curr test error

145a7c7

sijie reviewed Feb 11, 2019

View reviewed changes

ConcurrencyPractitioner added 2 commits February 11, 2019 18:24

Fixing nit comments

bbd59cb

Fixing false value

c795fab

ConcurrencyPractitioner added 2 commits February 12, 2019 18:35

Fixing silly comment

632c43a

Removing excess comment

c6b9bfa

ivankelly requested changes Feb 13, 2019

View reviewed changes

Fixing tests and addressing comment

b28e5c9

ivankelly approved these changes Feb 14, 2019

View reviewed changes

sijie approved these changes Feb 16, 2019

View reviewed changes

sijie merged commit 924f495 into apache:master Feb 16, 2019

jiazhai mentioned this pull request Aug 9, 2019

Add more config for auto-topic-creation #4926

Closed

fxbing mentioned this pull request Aug 15, 2019

Add more config for auto-topic-creation #4963

Merged

sijie mentioned this pull request Aug 18, 2019

Add configuration properties to limit the automatic creation of topics. #3436

Closed

Adding configuration to disable auto-topic creation #3450

Adding configuration to disable auto-topic creation #3450

Uh oh!

Conversation

ConcurrencyPractitioner commented Jan 27, 2019

Feature Request

Documentation

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

ConcurrencyPractitioner commented Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sijie commented Jan 27, 2019

Uh oh!

ConcurrencyPractitioner commented Jan 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ConcurrencyPractitioner commented Jan 28, 2019

Uh oh!

sijie commented Jan 28, 2019

Uh oh!

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

ConcurrencyPractitioner commented Feb 4, 2019

Uh oh!

sijie commented Feb 4, 2019

Uh oh!

ConcurrencyPractitioner commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sijie commented Feb 6, 2019

Uh oh!

ConcurrencyPractitioner commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sijie commented Feb 6, 2019

Uh oh!

ConcurrencyPractitioner commented Feb 10, 2019

Uh oh!

ConcurrencyPractitioner commented Feb 11, 2019

Uh oh!

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

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!

ConcurrencyPractitioner commented Feb 12, 2019

Uh oh!

sijie commented Feb 13, 2019

Uh oh!

ConcurrencyPractitioner commented Feb 13, 2019

ConcurrencyPractitioner commented Jan 27, 2019 •

edited

Loading

ConcurrencyPractitioner commented Jan 27, 2019 •

edited

Loading

ConcurrencyPractitioner commented Feb 6, 2019 •

edited

Loading

ConcurrencyPractitioner commented Feb 6, 2019 •

edited

Loading