Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pulsar-broker] Implement AutoSubscriptionCreation by namespace override #6637

Merged
merged 3 commits into from
Apr 14, 2020

Conversation

murong00
Copy link
Contributor

Motivation

Add a new namespace policy autoSubscriptionCreationOverride which will enable an override of broker autoSubscriptionCreation settings on the namespace level. Users can keep autoSubscriptionCreation disabled for the broker and allow it on a specific namespace using this feature.

Modifications

  • Add a new namespace policy: autoSubscriptionCreationOverride and associated API / CLI interface for setting and removing.
  • When checking autoSubscriptionCreation configuration, the broker first retrieves namespace policies from zookeeper. If not set, it falls back to broker configuration.
  • Some minor improvement on autoTopicCreationOverride (e.g. v1 Namespaces API & cli md)

@jiazhai
Copy link
Member

jiazhai commented Mar 31, 2020

@murong00 thanks for this feature. Please check the CI failures.

@jiazhai
Copy link
Member

jiazhai commented Apr 2, 2020

/pulsarbot run-failure-checks

1 similar comment
@jiazhai
Copy link
Member

jiazhai commented Apr 2, 2020

/pulsarbot run-failure-checks

Comment on lines 669 to 670
globalZk().setData(path(POLICIES, namespaceName.toString()),
jsonMapper().writeValueAsBytes(policiesNode.getKey()), policiesNode.getValue().getVersion());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's better to call setData() asynchronously.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 717 to 718
globalZk().setData(path(POLICIES, namespaceName.toString()),
jsonMapper().writeValueAsBytes(policiesNode.getKey()), policiesNode.getValue().getVersion());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the above comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@murong00
Copy link
Contributor Author

murong00 commented Apr 7, 2020

@codelipenghui I hava addressed your comment, please help to take a look, thanks.

@murong00
Copy link
Contributor Author

murong00 commented Apr 7, 2020

/pulsarbot run-failure-checks

1 similar comment
@murong00
Copy link
Contributor Author

murong00 commented Apr 8, 2020

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codelipenghui codelipenghui self-requested a review April 8, 2020 10:56
Comment on lines 647 to 648
log.info("[{}] Successfully removed autoTopicCreation override on namespace {}", clientAppId(), namespaceName);
asyncResponse.resume(Response.noContent().build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response should complete in the zookeeper callback. Otherwise, the broker always returns success.

Copy link
Contributor Author

@murong00 murong00 Apr 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codelipenghui Thanks for pointing out this, have fixed.

jsonMapper().writeValueAsBytes(policiesNode.getKey()), policiesNode.getValue().getVersion(),
(rc, path1, ctx, stat) -> {
if (rc == KeeperException.Code.OK.intValue()) {
policiesCache().invalidate(path(POLICIES, namespaceName.toString()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks should complete the asyncResponse here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 594 to 595
String autoOverride = autoTopicCreationOverride.allowAutoTopicCreation ? "enabled" : "disabled";
log.info("[{}] Successfully {} autoTopicCreation on namespace {}", clientAppId(), autoOverride, namespaceName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should move to the zookeeper callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 699 to 701
String autoOverride = autoSubscriptionCreationOverride.allowAutoSubscriptionCreation ? "enabled" : "disabled";
log.info("[{}] Successfully {} autoSubscriptionCreation on namespace {}", clientAppId(), autoOverride, namespaceName);
asyncResponse.resume(Response.noContent().build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should move to the zookeeper callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 752 to 753
log.info("[{}] Successfully removed autoSubscriptionCreation override on namespace {}", clientAppId(), namespaceName);
asyncResponse.resume(Response.noContent().build());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should move to the zookeeper callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@murong00
Copy link
Contributor Author

murong00 commented Apr 9, 2020

/pulsarbot run-failure-checks

1 similar comment
@murong00
Copy link
Contributor Author

murong00 commented Apr 9, 2020

/pulsarbot run-failure-checks

@murong00
Copy link
Contributor Author

murong00 commented Apr 9, 2020

/pulsarbot run-failure-checks

@sijie sijie added the doc-required Your PR changes impact docs and you will update later. label Apr 9, 2020
@sijie
Copy link
Member

sijie commented Apr 9, 2020

/pulsarbot run-failure-checks

1 similar comment
@sijie
Copy link
Member

sijie commented Apr 12, 2020

/pulsarbot run-failure-checks

@codelipenghui
Copy link
Contributor

@murong00 Could you please rebase your branch since some test-related fixes merged into the master branch.

@murong00
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie sijie merged commit d8be7c5 into apache:master Apr 14, 2020
@Anonymitaet Anonymitaet removed the doc-required Your PR changes impact docs and you will update later. label Jun 10, 2020
@Anonymitaet
Copy link
Member

huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
…ide (apache#6637)

### Motivation

Add a new namespace policy `autoSubscriptionCreationOverride` which will enable an override of broker `autoSubscriptionCreation` settings on the namespace level. Users can keep `autoSubscriptionCreation` disabled for the broker and allow it on a specific namespace using this feature.

### Modifications

* Add a new namespace policy: `autoSubscriptionCreationOverride` and associated API / CLI interface for setting and removing.
* When checking `autoSubscriptionCreation` configuration, the broker first retrieves namespace policies from zookeeper. If not set, it falls back to broker configuration. 
* Some minor improvement on `autoTopicCreationOverride` (e.g. v1 Namespaces API & cli md)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants