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

[docs]Add topic-level policy config #9108

Merged
merged 5 commits into from
Feb 1, 2021

Conversation

Jennifer88huang-zz
Copy link
Contributor

@Jennifer88huang-zz Jennifer88huang-zz commented Jan 2, 2021

Motivation

Topic-level policies are available since Pulsar 2.7.0.

Modifications

Add how to configure topic-level policies.

Related to #9161

@Jennifer88huang-zz Jennifer88huang-zz self-assigned this Jan 2, 2021
@Jennifer88huang-zz Jennifer88huang-zz changed the title [WIP][docs]Add topic-level policy config [docs]Add topic-level policy config Jan 10, 2021
@Jennifer88huang-zz Jennifer88huang-zz added this to the 2.8.0 milestone Jan 10, 2021
@Jennifer88huang-zz Jennifer88huang-zz added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. component/topic-policy release/2.7.1 labels Jan 10, 2021
Copy link
Contributor

@Huanli-Meng Huanli-Meng left a comment

Choose a reason for hiding this comment

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

Leave my comments. PTAL


Usage
```bash
$ pulsar-admin topics subcommand
```

From Pulsar 2.7.0, some namespace level policies are available on topic level. To enable topic level policy in Pulsar, you need to configure the following parameters in the `broker.conf` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
From Pulsar 2.7.0, some namespace level policies are available on topic level. To enable topic level policy in Pulsar, you need to configure the following parameters in the `broker.conf` file.
From Pulsar 2.7.0, some namespace-level policies are available on topic level. To enable topic-level policies in Pulsar, you need to configure the following parameters in the `broker.conf` file.

same comments for docs of other releases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thank you.

Copy link
Contributor

@freeznet freeznet left a comment

Choose a reason for hiding this comment

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

Please refer to my comments, some commands name are not aligned with code.

@@ -1815,6 +1822,36 @@ Subcommands
* `get-deduplication`
* `set-deduplication`
* `remove-deduplication`
* `get-max-producers`
Copy link
Contributor

Choose a reason for hiding this comment

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

according to code, this should be get-maxProducers, set-maxProducers and remove-maxProducers

* `get-offload-policies`
* `set-offload-policies`
* `remove-offload-policies`
* `get-max-unacked-messages-per-subscription`
Copy link
Contributor

Choose a reason for hiding this comment

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

get-max-unacked-messages-on-subscription

* `set-offload-policies`
* `remove-offload-policies`
* `get-max-unacked-messages-per-subscription`
* `set-max-unacked-messages-per-subscription`
Copy link
Contributor

Choose a reason for hiding this comment

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

set-max-unacked-messages-on-subscription

* `remove-offload-policies`
* `get-max-unacked-messages-per-subscription`
* `set-max-unacked-messages-per-subscription`
* `remove-max-unacked-messages-per-subscription`
Copy link
Contributor

Choose a reason for hiding this comment

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

remove-max-unacked-messages-on-subscription

* `get-max-unacked-messages-per-subscription`
* `set-max-unacked-messages-per-subscription`
* `remove-max-unacked-messages-per-subscription`
* `get-max-unacked-messages-per-consumer`
Copy link
Contributor

Choose a reason for hiding this comment

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

get-max-unacked-messages-on-consumer

* `set-max-unacked-messages-per-subscription`
* `remove-max-unacked-messages-per-subscription`
* `get-max-unacked-messages-per-consumer`
* `set-max-unacked-messages-per-consumer`
Copy link
Contributor

Choose a reason for hiding this comment

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

set-max-unacked-messages-on-consumer

* `remove-max-unacked-messages-per-subscription`
* `get-max-unacked-messages-per-consumer`
* `set-max-unacked-messages-per-consumer`
* `remove-max-unacked-messages-per-consumer`
Copy link
Contributor

Choose a reason for hiding this comment

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

remove-max-unacked-messages-on-consumer

@Jennifer88huang-zz
Copy link
Contributor Author

Jennifer88huang-zz commented Jan 11, 2021

@freeznet Thank you for your comments. Is it better to keep consistent for them in the code?

image

@freeznet
Copy link
Contributor

@jennifer88huang I think all commands should in lower case and unified format, as for the blue blocks, like get-maxProducers, should changed to get-max-producers. For the red blocks, I think they are fine, since the command name is just same as the function name, like GetMaxConsumersPerSubscription -> get-max-consumers-per-subscription.

@codelipenghui
Copy link
Contributor

@freeznet I think get-maxProducers is mistaken in the previous PR. We can add get-max-producers command first and add @Deprecated for the non-lower-case commands. And we can also make the non-lower-case command does not appear while run bin/pulsar-admin topics. This will guarantee compatibility.

@Jennifer88huang-zz
Copy link
Contributor Author

Jennifer88huang-zz commented Jan 11, 2021

@freeznet @codelipenghui thank you for your confirmation.
For those in the red boxes, I want to say that in namespaces, we use the word "xxx-per-xxx" (see screenshot below). Since those topic-level polices come from the namespace level, is it more user-friendly if we keep consistent and use the word "per" for topic-level policies? Otherwise, users might use the wrong commands if they want to adopt namespace-level policies to topic-level polices. cc @sijia-w
image

@codelipenghui
Copy link
Contributor

@jennifer88huang No, topic policy always for one topic, so we don't need to add -per-topic

@Jennifer88huang-zz
Copy link
Contributor Author

@sijia-w confirmed with eng, we need to keep topic-level commands consistent with that in the namespace-level.
So it should be "get-max-producers", "get-max-unacked-messages-per-consumer", get-max-unacked-messages-per-subscription". Use this kind of format in the docs, and eng will refine the code accordingly.
cc @codelipenghui @freeznet

sijie pushed a commit that referenced this pull request Jan 26, 2021
#9215)


Fixes #9205
### Motivation
In #9108, we add some topic-level policies commands, and found some commands are not consistent with that for namespace-level.
For example,
On namespace-level, the policies commands are:
```
get-max-producers
set-max-producers
remove-max-producers
get-max-unacked-messages-per-subscription
set-max-unacked-messages-per-subscription
remove-max-unacked-messages-per-subscription
```
On topic-level, the polices commands are:
```
get-maxProducers
set-maxProducers
remove-maxProducers
get-max-unacked-messages-on-subscription
set-max-unacked-messages-on-subscription
remove-max-unacked-messages-on-subscription
```

### Modifications
Keep topic-level policies commands consistent with that for namespace
codelipenghui pushed a commit that referenced this pull request Jan 26, 2021
#9215)

Fixes #9205
In #9108, we add some topic-level policies commands, and found some commands are not consistent with that for namespace-level.
For example,
On namespace-level, the policies commands are:
```
get-max-producers
set-max-producers
remove-max-producers
get-max-unacked-messages-per-subscription
set-max-unacked-messages-per-subscription
remove-max-unacked-messages-per-subscription
```
On topic-level, the polices commands are:
```
get-maxProducers
set-maxProducers
remove-maxProducers
get-max-unacked-messages-on-subscription
set-max-unacked-messages-on-subscription
remove-max-unacked-messages-on-subscription
```

Keep topic-level policies commands consistent with that for namespace

(cherry picked from commit d557e0a)
@Jennifer88huang-zz
Copy link
Contributor Author

Thank you @315157973 for fixing the command name issues in #9215. Shall we go on with the doc PR? @freeznet

Copy link
Contributor

@freeznet freeznet left a comment

Choose a reason for hiding this comment

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

LGTM

@Jennifer88huang-zz
Copy link
Contributor Author

Thank you @freeznet
@315157973 @codelipenghui @Huanli-Meng do you have any concern on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.7.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants