BewareMyPower commented on code in PR #20884:
URL: https://github.com/apache/pulsar/pull/20884#discussion_r1425029203
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2514,22 +2514,26 @@ private void handleDynamicConfigurationUpdates() {
return;
}
Field configField = configFieldWrapper.field;
- Object newValue =
FieldParser.value(data.get(configKey), configField);
- if (configField != null) {
- Consumer listener =
configRegisteredListeners.get(configKey);
- try {
- Object existingValue =
configField.get(pulsar.getConfiguration());
+ Consumer listener =
configRegisteredListeners.get(configKey);
+ Object existingValue;
+
+ try {
+ final Object newValue;
+ if (configField != null) {
+ newValue =
FieldParser.value(data.get(configKey), configField);
+ existingValue =
configField.get(pulsar.getConfiguration());
configField.set(pulsar.getConfiguration(),
newValue);
- log.info("Successfully updated
configuration {}/{}", configKey,
- data.get(configKey));
- if (listener != null &&
!existingValue.equals(newValue)) {
- listener.accept(newValue);
- }
- } catch (Exception e) {
- log.error("Failed to update config {}/{}",
configKey, newValue);
+ } else {
+ newValue = value;
+ existingValue =
configFieldWrapper.customValue;
+ configFieldWrapper.customValue = newValue
==null ? null :String.valueOf(newValue);
}
- } else {
- log.error("Found non-dynamic field in
dynamicConfigMap {}/{}", configKey, newValue);
+ log.info("Successfully updated configuration
{}/{}", configKey, data.get(configKey));
+ if (listener != null &&
!Objects.equals(existingValue, newValue)) {
+ listener.accept(newValue);
+ }
+ } catch (Exception e) {
+ log.error("Failed to update config {}/{}",
configKey, newValue);
Review Comment:
```suggestion
log.error("Failed to update config {}",
configKey, e);
```
You need to remove the `newValue` after applying my suggestion. BTW, logging
the exception would be good.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiDynamicConfigurationsTest.java:
##########
@@ -69,4 +74,38 @@ public void TestDeleteInvalidDynamicConfiguration() {
}
}
}
+
+ @Test
+ public void testRegisterCustomDynamicConfiguration() throws
PulsarAdminException {
+ String key = "my-broker-config-key-1";
+ String invalidValue = "invalid-value";
+
+ // register
+ pulsar.getBrokerService().registerCustomDynamicConfiguration(key,
value -> !value.equals(invalidValue));
+ assertThrows(IllegalArgumentException.class,
+ () ->
pulsar.getBrokerService().registerCustomDynamicConfiguration(key, null));
+ Map<String, String> allDynamicConfigurations =
admin.brokers().getAllDynamicConfigurations();
+ assertThat(allDynamicConfigurations).doesNotContainKey(key);
+
+ // update with listener
+ AtomicReference<String> changeValue = new AtomicReference<>(null);
+ Consumer<String> stringConsumer = changeValue::set;
+ pulsar.getBrokerService().registerConfigurationListener(key,
stringConsumer);
+ String newValue = "my-broker-config-value-1";
+ admin.brokers().updateDynamicConfiguration(key, newValue);
Review Comment:
Actually we can immediate assert
`admin.brokers().getAllDynamicConfigurations()` has the `key -> newValue` pair
because the map is retrieved from metadata store. We only need an
`Awaitility.await()` for `changeValue` because it might not be updated in time.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiDynamicConfigurationsTest.java:
##########
@@ -69,4 +74,38 @@ public void TestDeleteInvalidDynamicConfiguration() {
}
}
}
+
+ @Test
+ public void testRegisterCustomDynamicConfiguration() throws
PulsarAdminException {
+ String key = "my-broker-config-key-1";
+ String invalidValue = "invalid-value";
+
+ // register
+ pulsar.getBrokerService().registerCustomDynamicConfiguration(key,
value -> !value.equals(invalidValue));
+ assertThrows(IllegalArgumentException.class,
+ () ->
pulsar.getBrokerService().registerCustomDynamicConfiguration(key, null));
+ Map<String, String> allDynamicConfigurations =
admin.brokers().getAllDynamicConfigurations();
+ assertThat(allDynamicConfigurations).doesNotContainKey(key);
+
+ // update with listener
+ AtomicReference<String> changeValue = new AtomicReference<>(null);
+ Consumer<String> stringConsumer = changeValue::set;
+ pulsar.getBrokerService().registerConfigurationListener(key,
stringConsumer);
+ String newValue = "my-broker-config-value-1";
+ admin.brokers().updateDynamicConfiguration(key, newValue);
+ Awaitility.await().untilAsserted(() -> {
+ Map<String, String> configurations =
admin.brokers().getAllDynamicConfigurations();
+
assertThat(configurations).containsKey(key).containsValue(newValue);
Review Comment:
```suggestion
assertEquals(allDynamicConfigurations.get(key), newValue);
```
The existing assertion might pass if `configurations` has "key -> another
value" and "another key -> newValue", right?
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminApiDynamicConfigurationsTest.java:
##########
@@ -69,4 +74,38 @@ public void TestDeleteInvalidDynamicConfiguration() {
}
}
}
+
+ @Test
+ public void testRegisterCustomDynamicConfiguration() throws
PulsarAdminException {
+ String key = "my-broker-config-key-1";
+ String invalidValue = "invalid-value";
+
+ // register
+ pulsar.getBrokerService().registerCustomDynamicConfiguration(key,
value -> !value.equals(invalidValue));
+ assertThrows(IllegalArgumentException.class,
+ () ->
pulsar.getBrokerService().registerCustomDynamicConfiguration(key, null));
+ Map<String, String> allDynamicConfigurations =
admin.brokers().getAllDynamicConfigurations();
+ assertThat(allDynamicConfigurations).doesNotContainKey(key);
+
+ // update with listener
+ AtomicReference<String> changeValue = new AtomicReference<>(null);
+ Consumer<String> stringConsumer = changeValue::set;
+ pulsar.getBrokerService().registerConfigurationListener(key,
stringConsumer);
Review Comment:
```suggestion
pulsar.getBrokerService().registerConfigurationListener(key,
changeValue::set);
```
Simplify the code. lambda is convenient mainly because it can be an
anonymous function.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java:
##########
@@ -2514,22 +2514,26 @@ private void handleDynamicConfigurationUpdates() {
return;
}
Field configField = configFieldWrapper.field;
- Object newValue =
FieldParser.value(data.get(configKey), configField);
- if (configField != null) {
- Consumer listener =
configRegisteredListeners.get(configKey);
- try {
- Object existingValue =
configField.get(pulsar.getConfiguration());
+ Consumer listener =
configRegisteredListeners.get(configKey);
+ Object existingValue;
+
+ try {
+ final Object newValue;
+ if (configField != null) {
Review Comment:
```suggestion
try {
final Object existingValue;
final Object newValue;
if (configField != null) {
```
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
