[Issue #5395][broker] Implement AutoTopicCreation by namespace override #6471

klevy-toast · 2020-03-04T20:18:26Z

Motivation

This change introduces a new namespace policy autoTopicCreationOverride, which will enable an override of broker autoTopicCreation settings on the namespace level. You may keep autoTopicCreation disabled for the broker and allow it on a specific namespace using this feature.

Modifications

Add new namespace policy: autoTopicCreationOverride and associated API / CLI interface for setting and removing. Defaults to non-partitioned type, but also allows partitioned topics.
Modifies BrokerService: when checking autoTopicCreation configuration, the broker first retrieves namespace policies from zookeeper. If the autoTopicCreationOverride policy exists for that namespace then it uses those settings. If not, falls back to broker configuration.
Slight refactor to move TopicType enum to pulsar-common and add autoTopicCreationOverride to pulsar-common.

Verifying this change

Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change added tests and can be verified as follows:

Extended BrokerServiceAutoTopicCreationTest to test the overriding behavior of the namespace policy including both non-partitioned & partitioned topics.
Created AutoTopicCreationOverrideTest to ensure policy validation only allows valid policies.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API: (yes: new POST and DELETE for /{tenant}/{namespace}/allowAutoTopicCreationOverride in order to set new namespace policy)
The schema: (don't know)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (yes: new POST and DELETE for /{tenant}/{namespace}/allowAutoTopicCreationOverride in order to set new namespace policy)
The admin cli options: (yes: new set-allow-auto-topic-creation-override and delete-allow-auto-topic-creation-override)
Anything that affects deployment: (no)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (JavaDocs)

klevy-toast · 2020-03-05T18:24:19Z

Hi @sijie, I am unsure if the documentation that I added is enough. Is there another place I should be adding docs? Could you point me to it? Thanks.

sijie · 2020-03-06T23:15:09Z

@jiazhai @codelipenghui can you help review this pull request?

codelipenghui

The changes looks good, Some suggestions on naming:

Is it better to named to set-topic-auto-creation-policy and get-topic-auto-creation-policy?

codelipenghui · 2020-03-07T03:37:43Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java

+    @ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission"),
+            @ApiResponse(code = 404, message = "Tenant or cluster or namespace doesn't exist"),
+            @ApiResponse(code = 400, message = "Invalid autoTopicCreation override") })
+    public void setAllowAutoTopicCreationOverride(@PathParam("tenant") String tenant, @PathParam("namespace") String namespace,


It's better to leverage with AsyncResponse, you can take a look https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java#L198.

codelipenghui · 2020-03-07T03:38:00Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java

+    @ApiOperation(value = "Remove override of broker's allowAutoTopicCreation in a namespace")
+    @ApiResponses(value = { @ApiResponse(code = 403, message = "Don't have admin permission"),
+            @ApiResponse(code = 404, message = "Tenant or cluster or namespace doesn't exist") })
+    public void removeAllowAutoTopicCreationOverride(@PathParam("tenant") String tenant, @PathParam("namespace") String namespace) {


Same as above comment.

klevy-toast · 2020-03-10T18:49:05Z

Thanks @codelipenghui . I made changes to use AsyncResponse, and renamed things to set-auto-topic-creation-override etc.. I didn't want to change it around to topic-auto-creation-override because the original setting is called allowAutoTopicCreation so I think it is more consistent to maintain that order. There is no get command, as I figured that you could just use the namespaces policies command instead.

codelipenghui

I also want to suggest something in the name. All the namespace level policies are overwritten the broker level configurations, so do we need to add overwrite to the method name?

codelipenghui · 2020-03-15T09:48:09Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java

+        validateAdminAccessForTenant(namespaceName.getTenant());
+        validatePoliciesReadOnlyAccess();
+
+        if (!AutoTopicCreationOverride.isValidOverride(autoTopicCreationOverride)) {
+            throw new RestException(Status.PRECONDITION_FAILED, "Invalid configuration for autoTopicCreationOverride");
+        }


We'd better complete the asyncResponse when there are RunTimeException occurs. Otherwise, the client just can get a 500 response but can't get any error messages.

codelipenghui · 2020-03-15T09:48:26Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java

+        validateAdminAccessForTenant(namespaceName.getTenant());
+        validatePoliciesReadOnlyAccess();


Same as above comment.

klevy-toast · 2020-03-16T16:41:09Z

@codelipenghui I don't quite understand your naming suggestion. The new setting is an override, and the methods have that in their names, so I think that it is clear that they are overriding the broker level setting.

I added catches in the top level call in order to complete the async response. Let me know if you have any more suggestions. Thanks

codelipenghui · 2020-03-21T04:33:29Z

/pulsarbot run-failure-checks

klevy-toast · 2020-03-24T18:26:58Z

fyi tests passing now @codelipenghui

codelipenghui · 2020-03-25T03:58:31Z

@klevy-toast Can you help fix the checkstyle issue? The master branch enables checkstyle for pulsar-admin.

klevy-toast · 2020-03-25T17:28:18Z

/pulsarbot run-failure-checks

sijie · 2020-03-26T07:00:05Z

@klevy-toast thank you so much for your contributions!

jiazhai · 2020-05-08T09:37:19Z

For merge conflict. mark as 2.5.2 and cherry-picked into branch-2.5

…de (#6471) Fixes #5395 This change introduces a new namespace policy `autoTopicCreationOverride`, which will enable an override of broker `autoTopicCreation` settings on the namespace level. You may keep `autoTopicCreation` disabled for the broker and allow it on a specific namespace using this feature. - Add new namespace policy: `autoTopicCreationOverride` and associated API / CLI interface for setting and removing. Defaults to non-partitioned type, but also allows partitioned topics. - Modifies BrokerService: when checking `autoTopicCreation` configuration, the broker first retrieves namespace policies from zookeeper. If the `autoTopicCreationOverride` policy exists for that namespace then it uses those settings. If not, falls back to broker configuration. - Slight refactor to move `TopicType` enum to pulsar-common and add `autoTopicCreationOverride` to pulsar-common. (cherry picked from commit fdc3a9b)

Anonymitaet · 2020-06-10T07:41:31Z

Docs have been added:

…override (apache#6471) Fixes apache#5395 ### Motivation This change introduces a new namespace policy `autoTopicCreationOverride`, which will enable an override of broker `autoTopicCreation` settings on the namespace level. You may keep `autoTopicCreation` disabled for the broker and allow it on a specific namespace using this feature. ### Modifications - Add new namespace policy: `autoTopicCreationOverride` and associated API / CLI interface for setting and removing. Defaults to non-partitioned type, but also allows partitioned topics. - Modifies BrokerService: when checking `autoTopicCreation` configuration, the broker first retrieves namespace policies from zookeeper. If the `autoTopicCreationOverride` policy exists for that namespace then it uses those settings. If not, falls back to broker configuration. - Slight refactor to move `TopicType` enum to pulsar-common and add `autoTopicCreationOverride` to pulsar-common.

PR apache#6471 introduced the possibility to set auto topic creation settings at the namespace level PR apache#6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in apache#6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation

) PR #6471 introduced the possibility to set auto topic creation settings at the namespace level PR #6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in #6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation

) PR #6471 introduced the possibility to set auto topic creation settings at the namespace level PR #6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in #6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation (cherry picked from commit 965a80f)

…che#12699) PR apache#6471 introduced the possibility to set auto topic creation settings at the namespace level PR apache#6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in apache#6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation (cherry picked from commit 965a80f) (cherry picked from commit a034d14)

…che#12699) PR apache#6471 introduced the possibility to set auto topic creation settings at the namespace level PR apache#6685 is a fix to take into account the auto topic creation setting when creating a subscription but it uses the broker configuration and doesn't see the namespace level setting that was introduced in apache#6471 This fix uses the method that fetches config overrides in ZK instead of the one from the broker configuration. This way the method of the pulsar-admin uses the value set for the namespace for auto-topic-creation

klevy-toast added 12 commits February 11, 2020 19:24

Add allowAutoTopicCreation option to policies and CLI

9e6ee25

Auto-create by namespace MVP

f60eeae

Refactor to allow partitioned topics, added tests

e960426

Merge branch 'master' into feature/autocreate-by-namespace

bb50b3b

Cleanup commented code, imports

0a92e59

Tweak override validation

8bd458f

More cleanup, documentation, tests

49f443c

Add default partition type

0752ccf

Add license to new pulsar-common files

ee3a3dc

Remove null check that could cause silent failure

454b8c0

Switch back to synchronous policy retreival

4f31397

Merge branch 'master' into feature/autocreate-by-namespace

d5aa45f

sijie assigned klevy-toast Mar 5, 2020

sijie added the area/broker label Mar 5, 2020

sijie added this to the 2.6.0 milestone Mar 5, 2020

sijie added the type/feature The PR added a new feature or issue requested a new feature label Mar 5, 2020

sijie requested review from codelipenghui, merlimat and jiazhai March 5, 2020 01:25

sijie added the doc-required Your PR changes impact docs and you will update later. label Mar 5, 2020

codelipenghui reviewed Mar 7, 2020

View reviewed changes

klevy-toast added 2 commits March 9, 2020 12:59

Merge branch 'master' into feature/autocreate-by-namespace

bee6179

Make API async, renaming

423562b

klevy-toast requested a review from codelipenghui March 10, 2020 18:49

codelipenghui reviewed Mar 15, 2020

View reviewed changes

Add clause to resume async response on exceptions

163811a

sijie approved these changes Mar 20, 2020

View reviewed changes

klevy-toast added 2 commits March 20, 2020 14:28

Merge branch 'master' into feature/autocreate-by-namespace

84f4e4a

Fix typo

102ccfe

Merge branch 'master' into feature/autocreate-by-namespace

bafdb70

Merge branch 'master' into feature/autocreate-by-namespace

221ddab

Fix pulsar-client-admin style

f237b22

sijie merged commit fdc3a9b into apache:master Mar 26, 2020

klevy-toast deleted the feature/autocreate-by-namespace branch March 26, 2020 15:48

jiazhai added the release/2.5.2 label May 8, 2020

Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Jun 10, 2020

cbornet mentioned this pull request Nov 9, 2021

Fix namespace policy override ignored when creating subscription #12699

Merged

4 tasks

jtudelag mentioned this pull request Nov 2, 2022

Allow to set autoTopicCreationOverride at PulsarNamespace object. streamnative/pulsar-resources-operator#68

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #5395][broker] Implement AutoTopicCreation by namespace override #6471

[Issue #5395][broker] Implement AutoTopicCreation by namespace override #6471

klevy-toast commented Mar 4, 2020

klevy-toast commented Mar 5, 2020

sijie commented Mar 6, 2020

codelipenghui left a comment

codelipenghui Mar 7, 2020

codelipenghui Mar 7, 2020

klevy-toast commented Mar 10, 2020 •

edited

codelipenghui left a comment

codelipenghui Mar 15, 2020

codelipenghui Mar 15, 2020

klevy-toast commented Mar 16, 2020

codelipenghui commented Mar 21, 2020

klevy-toast commented Mar 24, 2020

codelipenghui commented Mar 25, 2020

klevy-toast commented Mar 25, 2020

sijie commented Mar 26, 2020

jiazhai commented May 8, 2020 •

edited

Anonymitaet commented Jun 10, 2020

		validateAdminAccessForTenant(namespaceName.getTenant());
		validatePoliciesReadOnlyAccess();

[Issue #5395][broker] Implement AutoTopicCreation by namespace override #6471

[Issue #5395][broker] Implement AutoTopicCreation by namespace override #6471

Conversation

klevy-toast commented Mar 4, 2020

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

klevy-toast commented Mar 5, 2020

sijie commented Mar 6, 2020

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Mar 7, 2020

Choose a reason for hiding this comment

codelipenghui Mar 7, 2020

Choose a reason for hiding this comment

klevy-toast commented Mar 10, 2020 • edited

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui Mar 15, 2020

Choose a reason for hiding this comment

codelipenghui Mar 15, 2020

Choose a reason for hiding this comment

klevy-toast commented Mar 16, 2020

codelipenghui commented Mar 21, 2020

klevy-toast commented Mar 24, 2020

codelipenghui commented Mar 25, 2020

klevy-toast commented Mar 25, 2020

sijie commented Mar 26, 2020

jiazhai commented May 8, 2020 • edited

Anonymitaet commented Jun 10, 2020

klevy-toast commented Mar 10, 2020 •

edited

jiazhai commented May 8, 2020 •

edited