Update Broker service configuration dynamically #186

rdhabalia · 2017-02-01T21:28:21Z

Motivation

as discussed at #184 : sometime it requires to update Broker-serviceConfiguration dynamically with the help of generic api.

Modifications

Annotation which defines service-configuration is dynamic
znode which contains map of dynamic serviceConfiguration's name and value
REST/AdminTool api to update specific dynamic-configuration by passing name/value
API to register listener which can be notified on config value change

Result

Broker can update serviceConfiguration dynamically and can execute appropriate action on configuration-value change.

merlimat

Change looks good. A couple of comments:

We should make very clear that the configuration in ZK overrides whatever is on the host config file. Eg: when we startup we should printing info log of what config we found on ZK
We should have 2 way of using dynamic fields
- Either we keep reading the value from ServiceConfiguration and thus at some point we pick up the updated value. We don't need mutexes, since there's no strict requirement on change visibility.
- Or we register the listener

merlimat · 2017-02-18T20:40:24Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/admin/Brokers.java

+     * @param configValue
+     *            : configuration value
+     */
+    private void updateServiecConfiguration(String configName, String configValue) {


Typo in updateServiecConfiguration

merlimat · 2017-02-18T22:02:46Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/admin/Brokers.java

+     */
+    private void updateServiecConfiguration(String configName, String configValue) {
+        try {
+            Field field = ServiceConfiguration.class.getDeclaredField(configName);


Instead of doing the reflection check on each request, could we doing just once (at startup) and keep a Set<> of keys that are dynamically updatable?

merlimat · 2017-02-18T22:29:27Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

@@ -62,6 +62,7 @@
    private long zooKeeperSessionTimeoutMillis = 30000;
    // Time to wait for broker graceful shutdown. After this time elapses, the
    // process will be killed
+    @FieldContext(dynamic = true)


I'd leave this PR to implement the "mechanism" and then we can make the actual variables dynamic in subsequent PRs.

Yes, but in order to write test-cases and perform testing of this feature I take one of this simple attribute in this PR. It helps to write e2e testcases for this feature.

merlimat · 2017-02-18T22:42:54Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/BrokerService.java

+
+    /**
+     * Update dynamic-ServiceConfiguration with value present into zk-configuraiton-map and register listeners on
+     * dynamic-ServiceConfiguration field to take appropriate action on change of zk-configuraiton-map.


zk-configuration-map

merlimat · 2017-02-18T22:43:25Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/BrokerService.java

+        // update brokerShutdownTimeoutMs value on listener notification
+        registerConfigurationListener("brokerShutdownTimeoutMs", new Consumer<String>() {
+            @Override
+            public void accept(String shutdownTime) {


can abbreviate into a lambda here

Actually, for this example, we shouldn't even need to register for notification, right?

We should make sure that the value ServiceConfiguration instance is updated, and then always read it from there, instead of caching it in a variable.

Actually, for this example, we shouldn't even need to register for notification, right?

Actually, we are not updating ServiceConfiguration instance's field value but we are calling registered listener for this field and it's registered-listener's responsibility to update the value and take appropriate action.
Because in case of #181 : Only when lookupRequestPermits field-value changes we want to take certain action such as update Semaphore's permits. Now, we have watch on entire dynamicConfigMap and listener can be triggered on every configKey-value change. So, registered listener can compare existing value and new value, and if it's changed then it can take appropriate action and update ServiceConfiguration.

addressed it on commit can you please take a look of it.

merlimat · 2017-02-18T22:59:29Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/BrokerService.java

+     * @param listener
+     *            : listener which takes appropriate action on config-value change
+     */
+    public void registerConfigurationListener(String configKey, Consumer<String> listener) {


Can we make the listener to be working for string, ints, longs? (Maybe leveraging the FieldParser)

yes, but we can have non-primitive ParameterizedTypes also for which we have to pass Field to parse the value. So, made appropriate change.

merlimat · 2017-02-18T23:00:21Z

pulsar-broker/src/main/java/com/yahoo/pulsar/broker/service/BrokerService.java

+                            field.setAccessible(true);
+                            field.set(pulsar().getConfiguration(), FieldParser.value(value,field));
+                            if (log.isDebugEnabled()) {
+                                log.debug("Successfully updated {}/{}", key, value);


We should print a info log line, for all the overridden variables

merlimat · 2017-02-18T23:01:14Z

pulsar-client-tools/src/main/java/com/yahoo/pulsar/admin/cli/CmdBrokers.java

    CmdBrokers(PulsarAdmin admin) {
        super("brokers", admin);
        jcommander.addCommand("list", new List());
        jcommander.addCommand("namespaces", new Namespaces());
+        jcommander.addCommand("updateConfig", new UpdateConfigurationCmd());


we've been using - as separator in other commands. eg: update-config

rdhabalia · 2017-02-22T07:13:08Z

@merlimat : addressed all the changes, added one more change:
ConfigRegistered should be triggered only if the configValue has been changed
any thought?

merlimat

Change looks very good, just a couple of minor comments

merlimat · 2017-02-28T20:05:36Z

pulsar-broker-common/src/main/java/com/yahoo/pulsar/broker/ServiceConfiguration.java

@@ -62,6 +62,7 @@
    private long zooKeeperSessionTimeoutMillis = 30000;
    // Time to wait for broker graceful shutdown. After this time elapses, the
    // process will be killed
+    @FieldContext(dynamic = true)


merlimat · 2017-02-28T20:07:52Z

pulsar-client-tools/src/main/java/com/yahoo/pulsar/admin/cli/CmdBrokers.java

@@ -48,9 +48,23 @@ void run() throws Exception {
        }
    }

+    @Parameters(commandDescription = "Update dynamic-serviceConfiguration of broker")


Can we also add a command to get the list of "updatable" config fields?

Also, it would be useful to have command to print all the config that are stored in ZK. To know what is being overridden.

added both commands:

getUpdatableConfigName

getAllConfigurations to get key,value for all configs

merlimat · 2017-02-28T20:09:41Z

pulsar-client-admin/src/main/java/com/yahoo/pulsar/client/admin/Brokers.java

+     * @param value
+     * @throws PulsarAdminException
+     */
+    void updateConfiguration(String configName, String configValue) throws PulsarAdminException;


Instead of "updating" configuration, in this case we would be effectively "overriding" the local broker configuration with a "more important" configuration stored in ZK

so, you mean we should rename the method to overrideDynamicConfiguration or we want to make any functionality change?

Just renaming. I'm not 100% sure about the name, just wanted to make clear that it doesn't update the config in a broker, rather it writes config in ZK that supersedes whatever all the brokers have locally.

updateDynamicConfigurationInZk, persistDynamicConfigValue, storeDynamicConfigValue ?

I have updated the document to call it out clearly and also renamed it as updateDynamicConfiguration. Do you think if we rename it to something else.?

merlimat · 2017-03-02T03:24:18Z

pulsar-broker/src/test/java/com/yahoo/pulsar/broker/admin/AdminApiTest.java

+        getPermitZkNodeMethod.setAccessible(true);
+        getPermitZkNodeMethod.invoke(pulsar.getBrokerService());
+        // verify value is updated
+        assertEquals(pulsar.getConfiguration().getBrokerShutdownTimeoutMs(), shutdownTime);


The notification from ZK might be delayed in the background thread. In addition since we're doing "dirty" reads (avoid synchronization), it might take a bit more after the new value is visible on the getter method. We should have a retry loop here in the test to give a bit of time.

I have added this logic in testUpdateDynamicConfigurationWithZkWatch.
This test-case directly triggers registerListener because pulsarService started when znode was not created and can't set the watch if node is not exist. so, in this testcase it explicitly triggers registerListener call.

Ok, tough is there also the other test that work with the zk watch and verifies we apply the change?

yes, testUpdateDynamicConfigurationWithZkWatch first creates znode and any further config change happens by zk-watch. I have added retry loop there to wait for refreshed value.

merlimat · 2017-03-02T03:24:59Z

pulsar-broker/src/test/java/com/yahoo/pulsar/broker/admin/AdminApiTest.java

+    public void testUpdateDynamicLocalConfiguration() throws Exception {
+
+        // (1) try to update dynamic field
+        final long shutdownTime = 10;


Can we verify that shutdown time is != 10 in the initial config?

… that

rdhabalia · 2017-03-03T23:47:58Z

@merlimat I have also added AdminTools documentation change in this PR.
As we have PR dependencies #274 -> #181 -> this PR. we can merge it if we are fine with the changes.

merlimat

👍

* Expose EventTime consistently as a non-pointer * Expanded docs

rdhabalia added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages type/feature The PR added a new feature or issue requested a new feature labels Feb 1, 2017

rdhabalia added this to the 1.17 milestone Feb 1, 2017

rdhabalia self-assigned this Feb 1, 2017

rdhabalia force-pushed the dyn_conf branch 7 times, most recently from e7ba703 to 206ba5d Compare February 3, 2017 19:14

merlimat reviewed Feb 18, 2017

View reviewed changes

rdhabalia force-pushed the dyn_conf branch 2 times, most recently from 3593728 to f4adac9 Compare February 21, 2017 02:31

merlimat reviewed Mar 2, 2017

View reviewed changes

rdhabalia force-pushed the dyn_conf branch 5 times, most recently from 224004d to 67a124e Compare March 3, 2017 22:15

rdhabalia added 3 commits March 3, 2017 14:49

Update Broker service configuration dynamically

54a8ca8

add configurationMap to avoid reflection everytime

a194aa9

Trigger listener on value change and update ServiceConfig value after…

cc3c92f

… that

rdhabalia force-pushed the dyn_conf branch 3 times, most recently from 691ccb0 to 012a47b Compare March 3, 2017 23:07

add getAllConfiguration adminApiCommand + add test-cases

bd5ce2c

rdhabalia force-pushed the dyn_conf branch from 012a47b to bd5ce2c Compare March 3, 2017 23:39

add dynamic-configuration command documentation

26c72f3

rdhabalia force-pushed the dyn_conf branch from 82059d9 to 26c72f3 Compare March 3, 2017 23:43

merlimat approved these changes Mar 5, 2017

View reviewed changes

merlimat merged commit 92b79d0 into apache:master Mar 5, 2017

rdhabalia mentioned this pull request Mar 6, 2017

Add server-side lookup throttling #181

Merged

rdhabalia mentioned this pull request Mar 24, 2017

Introduce new load manager API #303

Merged

rdhabalia deleted the dyn_conf branch June 21, 2017 18:54

sijie pushed a commit to sijie/pulsar that referenced this pull request Mar 4, 2018

during startup self.producer will always be null (apache#186)

3448c10

hrsakai pushed a commit to hrsakai/pulsar that referenced this pull request Dec 10, 2020

Expose EventTime consistently as a non-pointer (apache#186)

fc390a6

* Expose EventTime consistently as a non-pointer * Expanded docs

xiaotongwang1 mentioned this pull request Aug 4, 2021

Pulsar 2.7.0+ KOP 2.7.2.x getPartitionedTopicMetadata timeout #11532

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Broker service configuration dynamically #186

Update Broker service configuration dynamically #186

rdhabalia commented Feb 1, 2017

merlimat left a comment

merlimat Feb 18, 2017

merlimat Feb 18, 2017

merlimat Feb 18, 2017

rdhabalia Feb 21, 2017

merlimat Feb 28, 2017

merlimat Feb 18, 2017

merlimat Feb 18, 2017

merlimat Feb 18, 2017

rdhabalia Feb 21, 2017

rdhabalia Feb 21, 2017

merlimat Feb 18, 2017

rdhabalia Feb 21, 2017

merlimat Feb 18, 2017

merlimat Feb 18, 2017

rdhabalia commented Feb 22, 2017

merlimat left a comment

merlimat Feb 28, 2017

merlimat Feb 28, 2017

merlimat Mar 2, 2017

rdhabalia Mar 2, 2017

merlimat Feb 28, 2017

rdhabalia Mar 2, 2017 •

edited

Loading

merlimat Mar 3, 2017

rdhabalia Mar 3, 2017

rdhabalia Mar 3, 2017

merlimat Mar 2, 2017

rdhabalia Mar 2, 2017

merlimat Mar 3, 2017

rdhabalia Mar 3, 2017

merlimat Mar 2, 2017

rdhabalia commented Mar 3, 2017

merlimat left a comment

Update Broker service configuration dynamically #186

Update Broker service configuration dynamically #186

Conversation

rdhabalia commented Feb 1, 2017

Motivation

Modifications

Result

merlimat 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

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

Choose a reason for hiding this comment

rdhabalia commented Feb 22, 2017

merlimat 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

rdhabalia Mar 2, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdhabalia commented Mar 3, 2017

merlimat left a comment

Choose a reason for hiding this comment

rdhabalia Mar 2, 2017 •

edited

Loading