Skip to content

Conversation

@momo-jun
Copy link
Contributor

@momo-jun momo-jun commented Jan 14, 2022

As per #13077, the following changes were implemented in the docs:

  1. Add a comprehensive list of deprecated parameters under Broker configurations.
    image

  2. Change the following two parameter names in .md files except “puslar-perf managed-ledger zookeeperServers”.

    • Replace zookeeperServers with metadataStoreUrl.
    • Replace configurationStoreServers with configurationMetadataStoreUrl.
  3. Add a prefix my- to ZK node name as well as client port number 2181.
    image

  • doc

1. Add a comprehensive list of deprecated parameters under Broker configurations.
2. Change the following two parameter names in .md files except “puslar-perf managed-ledger zookeeperServers”.
  * Replace “zookeeperServers” with “metadataStoreUrl”.
  * Replace “configurationStoreServers” with “configurationMetadataStoreUrl”
3. Add “my-” prefix to ZK node name and port number 2181.
@github-actions
Copy link

@momo-jun:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions
Copy link

@momo-jun:Thanks for providing doc info!

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jan 14, 2022
Copy link
Member

@Anonymitaet Anonymitaet left a comment

Choose a reason for hiding this comment

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

@momo-jun thanks for your contribution!

  1. can you modify the PR title? It should be the summary of your PR changes, thanks

  2. can you @ related engineers to review this PR?

|tlsEnabled| Use `webServicePortTls` and `brokerServicePortTls` instead. |false|
|replicationTlsEnabled| Enable TLS when talking with other clusters to replicate messages |false|
|subscriptionKeySharedEnable| Whether to enable the Key_Shared subscription.|true|
|zookeeperServers| Zookeeper quorum connection string. Use `metadataStoreUrl` instead. ||
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
|zookeeperServers| Zookeeper quorum connection string. Use `metadataStoreUrl` instead. ||
|zookeeperServers| Zookeeper quorum connection string. Use `metadataStoreUrl` instead. |N/A|

If no default value, does it make sense to add "N/A" to tell users it does not have a default value rather than empty (tells nothing)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Yu. Updated.

|replicationTlsEnabled| Enable TLS when talking with other clusters to replicate messages |false|
|subscriptionKeySharedEnable| Whether to enable the Key_Shared subscription.|true|
|zookeeperServers| Zookeeper quorum connection string. Use `metadataStoreUrl` instead. ||
|configurationStoreServers| Configuration store connection string (as a comma-separated list). Use `configurationMetadataStoreUrl` instead. ||
Copy link
Member

Choose a reason for hiding this comment

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

same 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.

Thanks Yu. Updated.

@momo-jun momo-jun changed the title Doc update for PR#13077 Rename two parameters about ZK and add a comprehensive list for deprecated parameters of broker Jan 17, 2022
@momo-jun
Copy link
Contributor Author

@momo-jun thanks for your contribution!

  1. can you modify the PR title? It should be the summary of your PR changes, thanks
  2. can you @ related engineers to review this PR?

Thanks Yu. Will do that.

@momo-jun
Copy link
Contributor Author

Hi @merlimat @nodece, can you please help review this PR for doc updates? Thanks a lot.

@momo-jun momo-jun changed the title Rename two parameters about ZK and add a comprehensive list for deprecated parameters of broker Rename two parameters about metadatastore (ZK free) and add a comprehensive list for deprecated parameters of broker Jan 18, 2022
@momo-jun momo-jun changed the title Rename two parameters about metadatastore (ZK free) and add a comprehensive list for deprecated parameters of broker [Doc] Rename two parameters about metadatastore (ZK free) and add a comprehensive list for deprecated parameters of broker Jan 18, 2022
zookeeperServers=zk-0,zk-1,zk-2
configurationStoreServers=zk-0:2184,zk-remote:2184
metadataStoreUrl=my-zk-0:2181,my-zk-1:2181,my-zk-2:2181
configurationMetadataStoreUrl=my-zk-0:2184,my-zk-remote:2184
Copy link
Member

@nodece nodece Jan 20, 2022

Choose a reason for hiding this comment

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

No need to update this file.

Copy link
Member

Choose a reason for hiding this comment

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

Once #13777 will be merged to master, you can ignore this review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once #13777 will be merged to master, you can ignore this review.

Agree. Thanks for pointing this out.

```Yaml
clientAuthenticationPlugin: org.apache.pulsar.client.impl.auth.AuthenticationToken
clientAuthenticationParameters: file:///etc/pulsar/token/admin-token.txt
configurationStoreServers: zookeeper-cluster:2181 # auth requires a connection to zookeeper
Copy link
Member

@nodece nodece Jan 20, 2022

Choose a reason for hiding this comment

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

No need to update this file.

Copy link
Member

Choose a reason for hiding this comment

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

Once #13782 will be merged to master, you can ignore this review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once #13782 will be merged to master, you can ignore this review.

Agree. Thanks for pointing this out.

authorizationEnabled: true
authorizationProvider: org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider
configurationStoreServers: <configuration-store-servers>
configurationMetadataStoreUrl: <configuration-store-servers>
Copy link
Member

Choose a reason for hiding this comment

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

No need to update this file.

Copy link
Member

Choose a reason for hiding this comment

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

Once #13782 will be merged to master, you can ignore this review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once #13782 will be merged to master, you can ignore this review.

Agree. Thanks for pointing this out.

The [Pulsar proxy](concepts-architecture-overview.md#pulsar-proxy) can be configured in the `conf/proxy.conf` file.


|Name|Description|Default|
Copy link
Member

Choose a reason for hiding this comment

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

This section does not need to be updated.

Copy link
Member

Choose a reason for hiding this comment

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

Once #13777 will be merged to master, you can ignore this review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once #13777 will be merged to master, you can ignore this review.

Agree. Thanks for pointing this out.

@Anonymitaet Anonymitaet added this to the 2.10.0 milestone Jan 24, 2022
@Anonymitaet Anonymitaet changed the title [Doc] Rename two parameters about metadatastore (ZK free) and add a comprehensive list for deprecated parameters of broker [DO NOT MERGE][Doc] Rename two parameters about metadatastore (ZK free) and add a comprehensive list for deprecated parameters of broker Jan 24, 2022
@Anonymitaet
Copy link
Member

@momo-jun can you resolve the conflicts in this PR?

@momo-jun
Copy link
Contributor Author

momo-jun commented Feb 7, 2022

@momo-jun can you resolve the conflicts in this PR?

@Anonymitaet I've resolved the conflicts with PR#14081.
Hi @RobertIndie, can you pls help review the modifications? Thanks.

Co-authored-by: Zike Yang <zkyang@streamnative.io>
Copy link
Contributor Author

@momo-jun momo-jun left a comment

Choose a reason for hiding this comment

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

Add a description to explain the replacement for subscriptionKeySharedEnable as follows.
Use subscriptionTypesEnabled instead.

@Anonymitaet Anonymitaet merged commit 32700c9 into apache:master Feb 8, 2022
Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022
…omprehensive list for deprecated parameters of broker (apache#13758)
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants