You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2020/03/04 20:18:28 UTC
[GitHub] [pulsar] klevy-toast opened a new pull request #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
klevy-toast opened a new pull request #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471
Fixes #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.
### Verifying this change
- [x] 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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-601993254
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on issue #6471: [Issue #5395][broker]
Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
sijie commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-596004276
@jiazhai @codelipenghui can you help review this pull request?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie merged pull request #6471: [Issue #5395][broker]
Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] sijie commented on issue #6471: [Issue #5395][broker]
Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
sijie commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-604263903
@klevy-toast thank you so much for your contributions!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-595376272
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-600208081
@codelipenghui do you think that the object `AutoTopicCreationOverride` object should simply be `AutoTopicCreation`? or `AutoTopicCreationSettings`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on a change in pull request #6471:
[Issue #5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#discussion_r389224168
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
##########
@@ -298,6 +299,28 @@ public void modifyDeduplication(@PathParam("tenant") String tenant, @PathParam("
internalModifyDeduplication(enableDeduplication);
}
+ @POST
+ @Path("/{tenant}/{namespace}/allowAutoTopicCreationOverride")
+ @ApiOperation(value = "Override broker's allowAutoTopicCreation setting for 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"),
+ @ApiResponse(code = 400, message = "Invalid autoTopicCreation override") })
+ public void setAllowAutoTopicCreationOverride(@PathParam("tenant") String tenant, @PathParam("namespace") String namespace,
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on a change in pull request #6471:
[Issue #5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#discussion_r392657557
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
##########
@@ -554,6 +555,105 @@ protected void internalSetNamespaceMessageTTL(int messageTTL) {
}
}
+ protected void internalSetAutoTopicCreationOverride(AsyncResponse asyncResponse, AutoTopicCreationOverride autoTopicCreationOverride) {
+ validateAdminAccessForTenant(namespaceName.getTenant());
+ validatePoliciesReadOnlyAccess();
+
+ if (!AutoTopicCreationOverride.isValidOverride(autoTopicCreationOverride)) {
+ throw new RestException(Status.PRECONDITION_FAILED, "Invalid configuration for autoTopicCreationOverride");
+ }
+
+ // Force to read the data s.t. the watch to the cache content is setup.
+ policiesCache().getWithStatAsync(path(POLICIES, namespaceName.toString())).thenApply(
+ policies -> {
+ if (policies.isPresent()) {
+ Entry<Policies, Stat> policiesNode = policies.get();
+ policiesNode.getKey().autoTopicCreationOverride = autoTopicCreationOverride;
+ try {
+ // Write back the new policies into zookeeper
+ globalZk().setData(path(POLICIES, namespaceName.toString()),
+ jsonMapper().writeValueAsBytes(policiesNode.getKey()), policiesNode.getValue().getVersion());
+ policiesCache().invalidate(path(POLICIES, namespaceName.toString()));
+ asyncResponse.resume(Response.noContent().build());
+ log.info("[{}] Successfully {} on namespace {}", clientAppId(),
+ autoTopicCreationOverride.allowAutoTopicCreation ? "enabled" : "disabled", namespaceName);
+ return null;
+ } catch (KeeperException.NoNodeException e) {
+ log.error("[{}] Failed to modify autoTopicCreation status for namespace {}: does not exist", clientAppId(),
+ namespaceName);
+ asyncResponse.resume(new RestException(Status.NOT_FOUND, "Namespace does not exist"));
+ return null;
+ } catch (KeeperException.BadVersionException e) {
+ log.error(
+ "[{}] Failed to modify autoTopicCreation status on namespace {} expected policy node version={} : concurrent modification",
+ clientAppId(), namespaceName, policiesNode.getValue().getVersion());
+
+ asyncResponse.resume(new RestException(Status.CONFLICT, "Concurrent modification"));
+ return null;
+ } catch (Exception e) {
+ log.error("[{}] Failed to modify autoTopicCreation status on namespace {}", clientAppId(), namespaceName, e);
+ asyncResponse.resume(new RestException(e));
+ return null;
+ }
+ } else {
+ asyncResponse.resume(new RestException(Status.NOT_FOUND, "Namespace " + namespaceName + " does not exist"));
+ return null;
+ }
+ }
+ ).exceptionally(e -> {
+ log.error("[{}] Failed to modify autoTopicCreation status on namespace {}", clientAppId(), namespaceName, e);
+ asyncResponse.resume(new RestException(e));
+ return null;
+ });
+ }
+
+ protected void internalRemoveAutoTopicCreationOverride(AsyncResponse asyncResponse) {
+ validateAdminAccessForTenant(namespaceName.getTenant());
+ validatePoliciesReadOnlyAccess();
Review comment:
Same as above comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on a change in pull request #6471:
[Issue #5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#discussion_r389224183
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java
##########
@@ -298,6 +299,28 @@ public void modifyDeduplication(@PathParam("tenant") String tenant, @PathParam("
internalModifyDeduplication(enableDeduplication);
}
+ @POST
+ @Path("/{tenant}/{namespace}/allowAutoTopicCreationOverride")
+ @ApiOperation(value = "Override broker's allowAutoTopicCreation setting for 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"),
+ @ApiResponse(code = 400, message = "Invalid autoTopicCreation override") })
+ public void setAllowAutoTopicCreationOverride(@PathParam("tenant") String tenant, @PathParam("namespace") String namespace,
+ AutoTopicCreationOverride autoTopicCreationOverride) {
+ validateNamespaceName(tenant, namespace);
+ internalSetAllowAutoTopicCreationOverride(autoTopicCreationOverride);
+ }
+
+ @DELETE
+ @Path("/{tenant}/{namespace}/allowAutoTopicCreationOverride")
+ @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) {
Review comment:
Same as above comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-603429951
fyi tests passing now @codelipenghui
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-603626965
@klevy-toast Can you help fix the checkstyle issue? The master branch enables checkstyle for pulsar-admin.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-599819569
@codelipenghui oh, so you mean call them `set-auto-topic-creation` etc?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on a change in pull request #6471:
[Issue #5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on a change in pull request #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#discussion_r392657536
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java
##########
@@ -554,6 +555,105 @@ protected void internalSetNamespaceMessageTTL(int messageTTL) {
}
}
+ protected void internalSetAutoTopicCreationOverride(AsyncResponse asyncResponse, AutoTopicCreationOverride autoTopicCreationOverride) {
+ validateAdminAccessForTenant(namespaceName.getTenant());
+ validatePoliciesReadOnlyAccess();
+
+ if (!AutoTopicCreationOverride.isValidOverride(autoTopicCreationOverride)) {
+ throw new RestException(Status.PRECONDITION_FAILED, "Invalid configuration for autoTopicCreationOverride");
+ }
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-599817739
@klevy-toast I mean all namespace level policies are overwritten the broker level configuration. So I think we don't need to add `overwrite` to all namespace level methods, this is also consistent with the style of the current namespace level methods
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-597252798
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 thought that you could just use the `namespaces policies` command instead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-599638259
@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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-603977455
/pulsarbot run-failure-checks
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] codelipenghui commented on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-599844761
@klevy-toast Yes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [pulsar] klevy-toast edited a comment on issue #6471: [Issue
#5395][broker] Implement AutoTopicCreation by namespace override
Posted by GitBox <gi...@apache.org>.
klevy-toast edited a comment on issue #6471: [Issue #5395][broker] Implement AutoTopicCreation by namespace override
URL: https://github.com/apache/pulsar/pull/6471#issuecomment-597252798
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services