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 2022/07/09 08:06:35 UTC
[GitHub] [pulsar] tisonkun opened a new pull request, #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
tisonkun opened a new pull request, #16492:
URL: https://github.com/apache/pulsar/pull/16492
### Motivation
Cleanup code when reading it.
### Modifications
Encapsulate usage of deprecated configs:
* subscriptionKeySharedEnable
* zookeeperServers
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
- [x] `doc`
Configuration document may change, reflecting deprecations.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1185195140
Hit flaky test. Reported at https://github.com/apache/pulsar/issues/16558.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1186790732
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1185093854
/pulsarbot rerun-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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1179516592
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#discussion_r918775042
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2779,6 +2781,10 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private Set<String> additionalServlets = new TreeSet<>();
+ public boolean isSubscriptionKeySharedEnable() {
+ return subscriptionKeySharedEnable && subscriptionTypesEnabled.contains("Key_Shared");
+ }
Review Comment:
@codelipenghui I'll push a commit to add a UT along with this patch later. Please confirm whether it's proper when you have spare time :)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1188594111
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#discussion_r917475379
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2779,6 +2781,10 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private Set<String> additionalServlets = new TreeSet<>();
+ public boolean isSubscriptionKeySharedEnable() {
+ return subscriptionKeySharedEnable && subscriptionTypesEnabled.contains("Key_Shared");
+ }
Review Comment:
Here should be a bug fix. If the subscriptionKeySharedEnable = true but the subscriptionTypesEnabled doesn't have the `Key_Shared` subscription, we should treat it as Key_Shared subscription disabled.
https://github.com/apache/pulsar/blob/b68415a89dcd067511d01956447d1a507865534c/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java#L704-L708
Could you please add a UT?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1188471851
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 merged pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
hangc0276 merged PR #16492:
URL: https://github.com/apache/pulsar/pull/16492
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1185243846
/pulsarbot rerun-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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#discussion_r917509180
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2779,6 +2781,10 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private Set<String> additionalServlets = new TreeSet<>();
+ public boolean isSubscriptionKeySharedEnable() {
+ return subscriptionKeySharedEnable && subscriptionTypesEnabled.contains("Key_Shared");
+ }
Review Comment:
I'm glad to do so. Shall I separate the bug fix into a new PR or join in this PR?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1188551895
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1185093979
Weird. Many test cases failed due to container failure.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1184322124
cc @hangc0276
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1185194891
/pulsarbot rerun-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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1186927580
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1188613306
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1186817288
ping @codelipenghui @hangc0276
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1179526085
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#discussion_r921041584
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2779,6 +2781,10 @@ public class ServiceConfiguration implements PulsarConfiguration {
)
private Set<String> additionalServlets = new TreeSet<>();
+ public boolean isSubscriptionKeySharedEnable() {
+ return subscriptionKeySharedEnable && subscriptionTypesEnabled.contains("Key_Shared");
+ }
Review Comment:
Updated. Please give it another pass of review.
cc @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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#discussion_r921330478
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2839,15 +2849,8 @@ public String getBookkeeperMetadataStoreUrl() {
} else {
// Fallback to same metadata service used by broker, adding the "metadata-store" to specify the BK
// metadata adapter
- String suffix;
- if (StringUtils.isNotBlank(metadataStoreUrl)) {
- suffix = metadataStoreUrl;
- } else {
- // Fallback to old setting
- // Note: chroot is not settable by using 'zookeeperServers' config.
- suffix = ZKMetadataStore.ZK_SCHEME_IDENTIFIER + zookeeperServers;
- }
- return "metadata-store:" + suffix + BookKeeperConstants.DEFAULT_ZK_LEDGERS_ROOT_PATH;
+ // Note: chroot is not settable by using 'zookeeperServers' config.
+ return "metadata-store:" + getMetadataStoreUrl() + BookKeeperConstants.DEFAULT_ZK_LEDGERS_ROOT_PATH;
Review Comment:
I'd prefer to add a commit to make `getMetadataStoreUrl` explicitly add `ZKMetadataStore.ZK_SCHEME_IDENTIFIER`. Updating...
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1186108955
@hangc0276 @codelipenghui All required tests are passed. Please take a look.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a diff in pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#discussion_r921285739
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2839,15 +2849,8 @@ public String getBookkeeperMetadataStoreUrl() {
} else {
// Fallback to same metadata service used by broker, adding the "metadata-store" to specify the BK
// metadata adapter
- String suffix;
- if (StringUtils.isNotBlank(metadataStoreUrl)) {
- suffix = metadataStoreUrl;
- } else {
- // Fallback to old setting
- // Note: chroot is not settable by using 'zookeeperServers' config.
- suffix = ZKMetadataStore.ZK_SCHEME_IDENTIFIER + zookeeperServers;
- }
- return "metadata-store:" + suffix + BookKeeperConstants.DEFAULT_ZK_LEDGERS_ROOT_PATH;
+ // Note: chroot is not settable by using 'zookeeperServers' config.
+ return "metadata-store:" + getMetadataStoreUrl() + BookKeeperConstants.DEFAULT_ZK_LEDGERS_ROOT_PATH;
Review Comment:
We should take ZKMetadataStore.ZK_SCHEME_IDENTIFIER prefix here
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on a diff in pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on code in PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#discussion_r921307348
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java:
##########
@@ -2839,15 +2849,8 @@ public String getBookkeeperMetadataStoreUrl() {
} else {
// Fallback to same metadata service used by broker, adding the "metadata-store" to specify the BK
// metadata adapter
- String suffix;
- if (StringUtils.isNotBlank(metadataStoreUrl)) {
- suffix = metadataStoreUrl;
- } else {
- // Fallback to old setting
- // Note: chroot is not settable by using 'zookeeperServers' config.
- suffix = ZKMetadataStore.ZK_SCHEME_IDENTIFIER + zookeeperServers;
- }
- return "metadata-store:" + suffix + BookKeeperConstants.DEFAULT_ZK_LEDGERS_ROOT_PATH;
+ // Note: chroot is not settable by using 'zookeeperServers' config.
+ return "metadata-store:" + getMetadataStoreUrl() + BookKeeperConstants.DEFAULT_ZK_LEDGERS_ROOT_PATH;
Review Comment:
The configuration will resolve empty scheme as ZK. Once we decide to break backward compatibility, we should remove all usage of `zookeeperServers` and this fallback logic will be remove also. Thus, I don't think we should explicit specify the identifier, or we should explicit add it in `getMetadataStoreUrl` 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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] tisonkun commented on pull request #16492: [improve][cleanup] Keep usage of deprecated configs inside ServiceConfiguration
Posted by GitBox <gi...@apache.org>.
tisonkun commented on PR #16492:
URL: https://github.com/apache/pulsar/pull/16492#issuecomment-1186094360
/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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org