PIP 45: Deprecate zookeeper settings #14147

codelipenghui · 2022-02-07T10:17:01Z

Motivation

To deprecate the zookeeper settings. PIP 45 introduced a unified metadata store API for Pulsar,
so we should make the metadata-related configuration start with metadataStore, not zooKeeper.

Modification

change zooKeeperSessionTimeoutMillis to metadataStoreSessionTimeoutMillis
change zooKeeperOperationTimeoutSeconds to metadataStoreOperationTimeoutSeconds
change zooKeeperCacheExpirySeconds to metadataStoreCacheExpirySeconds

Compatibility

Introduced set method for the deprecated configurations. If the broker read the configurations
from the old version config file, the configuration loader will use the set method to apply the
configured value to new configurations.

Tests

Add new tests to make sure the change will not introduce a regression.

To deprecate the zookeeper settings. PIP 45 introduced unified metadata store API for Pulsar, so we should make the metadata related configuration start with `metadataStore`, not `zooKeeper`. change `zooKeeperSessionTimeoutMillis` to `metadataStoreSessionTimeoutMillis` change `zooKeeperOperationTimeoutSeconds` to `metadataStoreOperationTimeoutSeconds` change `zooKeeperCacheExpirySeconds` to `metadataStoreCacheExpirySeconds` Introduced set method for the deprecated configurations. If the broker read the configurations from the old version config file, the configuration loader will use the set method to apply the configued value to new configurations. Add new test to make sure the change will not introduce regression.

Jason918 · 2022-02-07T10:38:13Z

pulsar-common/src/main/java/org/apache/pulsar/common/util/FieldParser.java

-                        f.set(obj, value(trim(v), f));
+                        try {
+                            Method method = Reflections.getSetMethod(obj.getClass(), f);
+                            method.invoke(obj, value(v, f));


v --> trim(v) ?

nodece

LGTM

hangc0276 · 2022-02-07T14:43:43Z

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

    @FieldContext(
            category = CATEGORY_SERVER,
-            doc = "ZooKeeper cache expiry time in seconds"
+            doc = "ZooKeeper cache expiry time in seconds. "


You'd better use deprecated = true in fieldContext instead of doc.

nicoloboschi · 2022-02-07T15:28:34Z

...mmon/src/test/java/org/apache/pulsar/common/configuration/PulsarConfigurationLoaderTest.java

+            testConfigFile.delete();
+        }
+        try (PrintWriter printWriter = new PrintWriter(new OutputStreamWriter(new FileOutputStream(testConfigFile)))) {
+            printWriter.println("zooKeeperSessionTimeoutMillis=60");


can you add another test where the configuration contains both zooKeeper* and metadataStore* properties ?

@nicoloboschi I have changed the PR, could you please help review it again?

RobertIndie · 2022-02-08T01:45:43Z

pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java

@@ -1609,9 +1609,6 @@ public static WorkerConfig initializeWorkerConfigFromBrokerConfig(ServiceConfigu
        workerConfig.setAuthorizationEnabled(brokerConfig.isAuthorizationEnabled());
        workerConfig.setAuthorizationProvider(brokerConfig.getAuthorizationProvider());
        workerConfig.setConfigurationMetadataStoreUrl(brokerConfig.getConfigurationMetadataStoreUrl());
-        workerConfig.setZooKeeperSessionTimeoutMillis(brokerConfig.getZooKeeperSessionTimeoutMillis());
-        workerConfig.setZooKeeperOperationTimeoutSeconds(brokerConfig.getZooKeeperOperationTimeoutSeconds());
-


Seems that we need to set metadataStoreSessionTimeoutMillis etc. for the workerConfig here.

RobertIndie · 2022-02-08T01:51:06Z

...websocket/src/main/java/org/apache/pulsar/websocket/service/WebSocketProxyConfiguration.java

+    @FieldContext(doc = "ZooKeeper session timeout in milliseconds. "
+            + "@deprecated - Use metadataStoreSessionTimeoutMillis instead.")
    private long zooKeeperSessionTimeoutMillis = 30000;


Suggested change

@FieldContext(doc = "ZooKeeper session timeout in milliseconds. "

+ "@deprecated - Use metadataStoreSessionTimeoutMillis instead.")

private long zooKeeperSessionTimeoutMillis = 30000;

@Deprecated

@FieldContext(doc = "ZooKeeper session timeout in milliseconds. "

+ "@deprecated - Use metadataStoreSessionTimeoutMillis instead.",

deprecated = true)

private long zooKeeperSessionTimeoutMillis = 30000;

It's better to add deprecated here.

RobertIndie · 2022-02-08T01:51:36Z

...websocket/src/main/java/org/apache/pulsar/websocket/service/WebSocketProxyConfiguration.java

+    @FieldContext(doc = "ZooKeeper cache expiry time in seconds. "
+            + "@deprecated - Use metadataStoreCacheExpirySeconds instead.")
    private int zooKeeperCacheExpirySeconds = 300;


Suggested change

@FieldContext(doc = "ZooKeeper cache expiry time in seconds. "

+ "@deprecated - Use metadataStoreCacheExpirySeconds instead.")

private int zooKeeperCacheExpirySeconds = 300;

@Deprecated

@FieldContext(doc = "ZooKeeper cache expiry time in seconds. "

+ "@deprecated - Use metadataStoreCacheExpirySeconds instead.",

deprecated = true)

private int zooKeeperCacheExpirySeconds = 300;

It's better to add deprecated here.

codelipenghui · 2022-02-08T09:00:44Z

@nicoloboschi @Jason918 @RobertIndie @hangc0276 @nodece I have pushed new commits to make sure the change will not break running Pulsar in the Docker environment because we are using a script to change the config value. For the helm chart, it will use the new config file but with old config names, as @nicoloboschi mentioned both of the new configs and the deprecated configs will be present in the new config file. So I changed the default value of the deprecated configs to -1, the new config value only is applied while the old value is absent.

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java

eolivelli · 2022-02-08T16:52:05Z

pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyConfigurationTest.java

@@ -53,5 +59,55 @@ public void testConfigFileDefaults() throws Exception {
        }
    }

+    @Test
+    public void testBackwardCompatibility() throws IOException {


what about testZooKeeperConfigurationBackwardCompatibilty ?

I want to keep testBackwardCompatibility(), if we have other changes need to keep backward compatibility, we can just add items to the test, otherwise, we need many tests methods.

eolivelli

LGTM

nicoloboschi

+1, great work

* Deprecate zookeeper settings To deprecate the zookeeper settings. PIP 45 introduced unified metadata store API for Pulsar, so we should make the metadata related configuration start with `metadataStore`, not `zooKeeper`. change `zooKeeperSessionTimeoutMillis` to `metadataStoreSessionTimeoutMillis` change `zooKeeperOperationTimeoutSeconds` to `metadataStoreOperationTimeoutSeconds` change `zooKeeperCacheExpirySeconds` to `metadataStoreCacheExpirySeconds` Introduced set method for the deprecated configurations. If the broker read the configurations from the old version config file, the configuration loader will use the set method to apply the configued value to new configurations. Add new test to make sure the change will not introduce regression. * Apply comment * Apply comment * Apply comment * Apply comment * Apply comment.

codelipenghui added the doc-required Your PR changes impact docs and you will update later. label Feb 7, 2022

codelipenghui self-assigned this Feb 7, 2022

codelipenghui added this to the 2.10.0 milestone Feb 7, 2022

shoothzj approved these changes Feb 7, 2022

View reviewed changes

codelipenghui force-pushed the penghui/deprecate-zookeeper-settings branch from f2c1186 to dcea79d Compare February 7, 2022 10:22

codelipenghui requested review from merlimat, Jason918, 315157973, BewareMyPower, gaoran10 and hangc0276 February 7, 2022 10:24

codelipenghui changed the title ~~Deprecate zookeeper settings~~ PIP 45: Deprecate zookeeper settings Feb 7, 2022

Jason918 reviewed Feb 7, 2022

View reviewed changes

Apply comment

7c6d5cb

nodece approved these changes Feb 7, 2022

View reviewed changes

Jason918 approved these changes Feb 7, 2022

View reviewed changes

hangc0276 reviewed Feb 7, 2022

View reviewed changes

nicoloboschi requested changes Feb 7, 2022

View reviewed changes

RobertIndie reviewed Feb 8, 2022

View reviewed changes

Apply comment

50b5670

Jason918 reviewed Feb 8, 2022

View reviewed changes

pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java Outdated Show resolved Hide resolved

codelipenghui added 3 commits February 8, 2022 18:40

Apply comment

6cf9ce7

Apply comment

c9afb99

Apply comment.

308e197

315157973 approved these changes Feb 8, 2022

View reviewed changes

eolivelli reviewed Feb 8, 2022

View reviewed changes

mattisonchao approved these changes Feb 9, 2022

View reviewed changes

Jason918 approved these changes Feb 9, 2022

View reviewed changes

momo-jun mentioned this pull request Feb 9, 2022

[Doc] Deprecate 3 zookeeper parameters #14186

Merged

1 task

eolivelli approved these changes Feb 9, 2022

View reviewed changes

nicoloboschi approved these changes Feb 9, 2022

View reviewed changes

RobertIndie approved these changes Feb 9, 2022

View reviewed changes

merlimat approved these changes Feb 9, 2022

View reviewed changes

merlimat merged commit 3e06571 into apache:master Feb 9, 2022

Anonymitaet added doc-complete Your PR changes impact docs and the related docs have been already added. and removed doc-required Your PR changes impact docs and you will update later. labels Feb 10, 2022

codelipenghui deleted the penghui/deprecate-zookeeper-settings branch April 12, 2022 08:15

codelipenghui restored the penghui/deprecate-zookeeper-settings branch May 17, 2022 01:22

codelipenghui deleted the penghui/deprecate-zookeeper-settings branch May 17, 2022 01:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIP 45: Deprecate zookeeper settings #14147

PIP 45: Deprecate zookeeper settings #14147

codelipenghui commented Feb 7, 2022

Jason918 Feb 7, 2022

codelipenghui Feb 7, 2022

nodece left a comment

hangc0276 Feb 7, 2022

nicoloboschi Feb 7, 2022

codelipenghui Feb 9, 2022

RobertIndie Feb 8, 2022

RobertIndie Feb 8, 2022

RobertIndie Feb 8, 2022

codelipenghui commented Feb 8, 2022

eolivelli Feb 8, 2022

codelipenghui Feb 9, 2022

eolivelli left a comment

nicoloboschi left a comment

PIP 45: Deprecate zookeeper settings #14147

PIP 45: Deprecate zookeeper settings #14147

Conversation

codelipenghui commented Feb 7, 2022

Motivation

Modification

Compatibility

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodece left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codelipenghui commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

nicoloboschi left a comment

Choose a reason for hiding this comment