Skip to content

Conversation

@Anonymitaet
Copy link
Member

@Anonymitaet Anonymitaet commented Feb 24, 2021

Add docs to fix #3104, #8977, and #8976.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

thank you very much for providing this useful information !
I really missed it when I started using this feature.

I left some feedback, please take a look @Anonymitaet

pulsar-admin namespaces set-bookie-affinity-group options
```

For more information about the command and descriptions, see [here](http://pulsar.apache.org/tools/pulsar-admin/2.8.0-SNAPSHOT/#-em-set-bookie-affinity-group-em-).
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not have a pointer to a specific version 2.8.0-SNAPSHOT, probably for links to the Pulsar website itself we can use simply reletive references

Copy link
Member Author

@Anonymitaet Anonymitaet Feb 24, 2021

Choose a reason for hiding this comment

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

@eolivelli thanks for your suggestion. While the Pulsar website itself (Pulsar Admin CLI doc) does not contain this command and the information on the "Pulsar Admin CLI" page is far behind from pulsar-admin where contains the latest information.

Seems that there was an issue (create automatic workflow) to sync contents from pulsar-admin to the Pulsar website, but no more updates for a long time. I think right now the best choice is to point users to get the latest information and we can update the link once we have a better option.

**Example**

```shell
bin/pulsar-admin bookies set-bookie-rack \
Copy link
Contributor

Choose a reason for hiding this comment

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

we could cite this command
bin/pulsar-admin bookies list-bookies

that allows you to have the list of available bookies.

otherwise it is not clear to the user where to pick the bookie identifiers

this new command is available only from 2.8.0 anwards

Copy link
Member

Choose a reason for hiding this comment

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

Let's decouple the documentation change into two different pull requests. The documentation here can simply be copied to other releases to improve the documentation there. bin/pulsar-admin bookies list-bookies is a separate command introduced in 2.8.0. Let's not couple them together.

When people try to set bookie track, people already know which bookie to set the track information. The bookie can be found in many different ways. I don't think we should add bookies list-bookies to confuse people.

One note that Yu can add is that the set-bookie-rack is to specify the track information for only one bookie. If people want to specify the track for all bookies, they have to run the command for individual bookie. But they can put a script when starting a bookie to set its rack.

Copy link
Contributor

@eolivelli eolivelli Feb 24, 2021

Choose a reason for hiding this comment

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

The bookie can be found in many different ways

the only alternative way I know is with bin/bookkeeper shell listbookies -a

As I know how BK works it is easy for me to find a way to get the list of bookies, but I am not sure that the regular user knows how the Bookie identifies itself.

I am saying this because I had to help users in the past to search for the actual bookie id in their cluster

btw I have no strong opinion on this, we cannot cite that command.
Anyone who is using pulsar-admin bookies will easily find the 'list-bookies' command

Copy link
Member

Choose a reason for hiding this comment

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

@eolivelli - listbookies serves a different purpose for people to list the available bookies. People don't use listbookies to get the list of bookies and then use set-bookie-rack to set rack for individual bookie. People should have a startup script to call set-bookie-rack when a bookie is starting up. See my other comments with @Anonymitaet. Let's avoid giving misleading information on how people should use set-bookie-rack

Copy link
Contributor

Choose a reason for hiding this comment

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

@sijie I am fine with not telling about this command here in this document

For your interest pulsar-admin bookies list_bookies and bookkeeper shell listbookies -a
reports all of the existent bookies, in spite of their state.

bookkeeper shell listbookies -rw reports only the available bookies

@sijie sijie added this to the 2.8.0 milestone Feb 24, 2021
@sijie sijie added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Feb 24, 2021
**Example**

```shell
bin/pulsar-admin bookies set-bookie-rack \
Copy link
Member

Choose a reason for hiding this comment

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

Let's decouple the documentation change into two different pull requests. The documentation here can simply be copied to other releases to improve the documentation there. bin/pulsar-admin bookies list-bookies is a separate command introduced in 2.8.0. Let's not couple them together.

When people try to set bookie track, people already know which bookie to set the track information. The bookie can be found in many different ways. I don't think we should add bookies list-bookies to confuse people.

One note that Yu can add is that the set-bookie-rack is to specify the track information for only one bookie. If people want to specify the track for all bookies, they have to run the command for individual bookie. But they can put a script when starting a bookie to set its rack.


```shell
bin/pulsar-admin bookies set-bookie-rack \
--bookie 127.0.0.1:3181 \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--bookie 127.0.0.1:3181 \
--bookie 127.0.0.1:3181 \

127.0.0.1:3181 should be changed to bookie-id. The bookie id is in <hostname>:<port> format.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sijie thanks for your suggestions.

I think your note, --bookie bookie-id (the bookie id is in <hostname>:<port> format), and Explain what people should put in --primary. are "explanations" for the command pulsar-admin bookies set-bookie-rack, right?

Currently, both pulsar-admin bookies list-bookies and pulsar-admin bookies set-bookie-rack are not available on the pulsar-admin website, and it seems that we have the following bookie related commands but none of them are available on the pulsar-admin website.
image

Will these commands be shown on the pulsar-admin website? If so, I think the explanations talked above should be added to the command pulsar-admin bookies set-bookie-rackon the pulsar-admin website rather than here because users who want to use pulsar-admin commands would navigate to the pulsar-admin website to see descriptions, notes, and flags. And here are just example values.

For example, I added the descriptions for --primary-group and --secondary-group on the pulsar-admin website rather than every occurrence they are shown on Pulsar website docs.
image

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Those commands should show up. You might want to ping @tuteng to see why they don't show up.

Copy link
Member Author

@Anonymitaet Anonymitaet Feb 25, 2021

Choose a reason for hiding this comment

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

Sure, then I'll add explanations (note, --bookie bookie-id (the bookie id is in <hostname>:<port> format), and Explain what people should put in --primary.) on pulsar-admin website rather than here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @tuteng, we have the following bookie related commands but none of them are shown on the pulsar-admin website, could you please take a look? thanks
image

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @tuteng I've created an issue here: #9717

```shell
bin/pulsar-admin bookies set-bookie-rack \
--bookie 127.0.0.1:3181 \
--hostname 127.0.0.1:3181 \
Copy link
Member

Choose a reason for hiding this comment

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

ditto

--auto-failover-policy-type min_available \
--auto-failover-policy-params min_limit=1,usage_threshold=80 \
--namespaces my-tenant/my-namespace \
--primary 10.193.216.* my-cluster policy-name
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
--primary 10.193.216.* my-cluster policy-name
--primary 10.193.216.* my-cluster policy-name

Explain what people should put in --primary.

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.

a minor comment. PTAL

pulsar-admin ns-isolation-policy set options
```

For more information about the command and descriptions, see [here](http://pulsar.apache.org/tools/pulsar-admin/2.8.0-SNAPSHOT/#-em-set-em-).
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better not to specify the specific version of pulsar-admin

Copy link
Member Author

Choose a reason for hiding this comment

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

@Huanli-Meng thanks for your suggestion. While the Pulsar website itself (Pulsar Admin CLI doc) does not contain this command and the information on the "Pulsar Admin CLI" page is far behind from pulsar-admin where contains the latest information.

Seems that there was an issue (create automatic workflow) to sync contents from pulsar-admin to the Pulsar website, but no more updates for a long time. I think right now the best choice is to point users to get the latest information and we can update the link once we have a better option.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Anonymitaet I have the same concern here, it's better not to specify the version. You can use the general one http://pulsar.apache.org/tools/pulsar-admin/
or use variable for the version.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I've changed them to general links.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

@Anonymitaet Anonymitaet Feb 25, 2021

Choose a reason for hiding this comment

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

@jennifer88huang this variable does not work for all cases, since for minor releases, for example, 2.6.3, http://pulsar.apache.org/tools/pulsar-admin/ does not have 2.6.3-SNAPSHOT, so I change the specific link (http://pulsar.apache.org/tools/pulsar-admin/2.8.0-SNAPSHOT/#-em-set-em-) to a general link http://pulsar.apache.org/tools/pulsar-admin/. PTAL.

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.

[Feature request] Namespace/topic rack affinity

6 participants