This is an automated email from the ASF dual-hosted git repository.
adoroszlai pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/master by this push:
new 8fb441b7d6 HDDS-8760. Let reconfiguration handler update
reconfigurable config objects (#4851)
8fb441b7d6 is described below
commit 8fb441b7d67a62feec6e45b6fc9d908074791889
Author: Doroszlai, Attila <[email protected]>
AuthorDate: Thu Jun 15 18:30:34 2023 +0200
HDDS-8760. Let reconfiguration handler update reconfigurable config objects
(#4851)
---
.../hadoop/hdds/conf/SimpleConfiguration.java | 54 +++++++++++++------
.../hdds/conf/SimpleConfigurationParent.java | 2 +-
.../hdds/conf/ConfigurationReflectionUtil.java | 62 +++++++++++++++++-----
.../hadoop/hdds/conf/ConfigurationSource.java | 3 +-
.../hadoop/hdds/conf/ReconfigurationHandler.java | 25 +++++++--
.../hdds/conf/TestReconfigurationHandler.java | 56 +++++++++++++++----
.../ozone/reconfig/TestOmReconfiguration.java | 5 +-
.../ozone/reconfig/TestScmReconfiguration.java | 3 +-
8 files changed, 160 insertions(+), 50 deletions(-)
diff --git
a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfiguration.java
b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfiguration.java
index 16fc4e6311..a48bbcdb3b 100644
---
a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfiguration.java
+++
b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfiguration.java
@@ -25,36 +25,52 @@ import java.util.concurrent.TimeUnit;
@ConfigGroup(prefix = "test.scm.client")
public class SimpleConfiguration extends SimpleConfigurationParent {
- @Config(key = "address", defaultValue = "localhost", description = "Client "
- + "address (To test string injection).", tags = ConfigTag.MANAGEMENT)
+ @Config(key = "address",
+ defaultValue = "localhost",
+ description = "Client address (To test string injection).",
+ tags = ConfigTag.MANAGEMENT)
private String clientAddress;
- @Config(key = "bind.host", defaultValue = "0.0.0.0", description = "Bind "
- + "host(To test string injection).", tags = ConfigTag.MANAGEMENT)
+ @Config(key = "bind.host",
+ defaultValue = "0.0.0.0",
+ description = "Bind host(To test string injection).",
+ tags = ConfigTag.MANAGEMENT)
private String bindHost;
- @Config(key = "compression.enabled", defaultValue = "true", description =
- "Compression enabled. (Just to test boolean flag)", tags =
- ConfigTag.MANAGEMENT)
+ @Config(key = "compression.enabled",
+ defaultValue = "true",
+ reconfigurable = true,
+ description = "Compression enabled. (Just to test boolean flag)",
+ tags = ConfigTag.MANAGEMENT)
private boolean compressionEnabled;
- @Config(key = "port", defaultValue = "9878", description = "Port number "
- + "config (To test in injection)", tags = ConfigTag.MANAGEMENT)
+ @Config(key = "port",
+ defaultValue = "9878",
+ description = "Port number config (To test int injection)",
+ tags = ConfigTag.MANAGEMENT)
private int port;
- @Config(key = "wait", type = ConfigType.TIME, timeUnit =
- TimeUnit.SECONDS, defaultValue = "30m", description = "Wait time (To "
- + "test TIME config type)", tags = ConfigTag.MANAGEMENT)
+ @Config(key = "wait",
+ type = ConfigType.TIME,
+ timeUnit = TimeUnit.SECONDS,
+ defaultValue = "30m",
+ reconfigurable = true,
+ description = "Wait time (To test TIME config type)",
+ tags = ConfigTag.MANAGEMENT)
private long waitTime;
- @Config(key = "class", type = ConfigType.CLASS,
- defaultValue = "java.lang.Object", description = "",
+ @Config(key = "class",
+ type = ConfigType.CLASS,
+ defaultValue = "java.lang.Object",
+ description = "",
tags = ConfigTag.OZONE)
private Class<?> myClass = Object.class;
- @Config(key = "threshold", type = ConfigType.DOUBLE,
- defaultValue = "10", description = "Threshold (To test DOUBLE config" +
- " type)", tags = ConfigTag.MANAGEMENT)
+ @Config(key = "threshold",
+ type = ConfigType.DOUBLE,
+ defaultValue = "10",
+ description = "Threshold (To test DOUBLE config type)",
+ tags = ConfigTag.MANAGEMENT)
private double threshold;
@PostConstruct
@@ -62,6 +78,10 @@ public class SimpleConfiguration extends
SimpleConfigurationParent {
if (port < 0) {
throw new NumberFormatException("Please use a positive port number");
}
+ if (waitTime < 42) {
+ throw new IllegalArgumentException("Wait time less than 42 seconds: " +
+ waitTime);
+ }
}
public void setClientAddress(String clientAddress) {
diff --git
a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfigurationParent.java
b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfigurationParent.java
index 85ef7babd7..04f2d5b686 100644
---
a/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfigurationParent.java
+++
b/hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/conf/SimpleConfigurationParent.java
@@ -20,7 +20,7 @@ package org.apache.hadoop.hdds.conf;
/**
* Parent class for the example configuration.
*/
-public class SimpleConfigurationParent {
+public class SimpleConfigurationParent extends ReconfigurableConfig {
@Config(key = "enabled", defaultValue = "true", description = "Example "
+ "boolean config.", tags = ConfigTag.MANAGEMENT)
diff --git
a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationReflectionUtil.java
b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationReflectionUtil.java
index 96e5546da2..d5c9587a2b 100644
---
a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationReflectionUtil.java
+++
b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationReflectionUtil.java
@@ -107,14 +107,26 @@ public final class ConfigurationReflectionUtil {
public static <T> void reconfigureProperty(T configuration, Field field,
String key, String value) {
+ Class<?> klass = field.getDeclaringClass();
if (!field.isAnnotationPresent(Config.class)) {
throw new ConfigurationException("Not configurable field: "
- + field.getDeclaringClass() + "." + field.getName());
+ + klass + "." + field.getName());
}
Config configAnnotation = field.getAnnotation(Config.class);
- setField(field.getDeclaringClass(), configuration, field, configAnnotation,
- key, value);
+ try {
+ final Object oldValue = forcedFieldGet(field, configuration);
+ setField(klass, configuration, field, configAnnotation, key, value);
+ try {
+ callPostConstruct(configuration);
+ } catch (Exception e) {
+ forcedFieldSet(field, configuration, oldValue);
+ throw e;
+ }
+ } catch (IllegalAccessException e) {
+ throw new ConfigurationException("Failed to inject configuration to "
+ + klass.getSimpleName() + "." + field.getName(), e);
+ }
}
private static <T> void setField(
@@ -135,19 +147,45 @@ public final class ConfigurationReflectionUtil {
}
/**
- * Set the value of one field even if it's private.
+ * Set possibly private {@code field} to {@code value} in {@code object}.
*/
private static <T> void forcedFieldSet(Field field, T object, Object value)
throws IllegalAccessException {
- boolean accessChanged = false;
+ final boolean accessChanged = setAccessible(field);
+ try {
+ field.set(object, value);
+ } finally {
+ if (accessChanged) {
+ field.setAccessible(false);
+ }
+ }
+ }
+
+ /**
+ * @return the value of possibly private {@code field} in {@code object}
+ */
+ private static <T> Object forcedFieldGet(Field field, T object)
+ throws IllegalAccessException {
+ final boolean accessChanged = setAccessible(field);
+ try {
+ return field.get(object);
+ } finally {
+ if (accessChanged) {
+ field.setAccessible(false);
+ }
+ }
+ }
+
+ /**
+ * Make {@code field} accessible.
+ * @return true if access changed
+ */
+ private static boolean setAccessible(Field field) {
if (!field.isAccessible()) {
field.setAccessible(true);
- accessChanged = true;
- }
- field.set(object, value);
- if (accessChanged) {
- field.setAccessible(false);
+ return true;
}
+ return false;
}
private static ConfigType detectConfigType(Field field) {
@@ -174,8 +212,8 @@ public final class ConfigurationReflectionUtil {
return type;
}
- public static <T> void callPostConstruct(Class<T> configurationClass,
- T configObject) {
+ static <T> void callPostConstruct(T configObject) {
+ Class<?> configurationClass = configObject.getClass();
for (Method method : configurationClass.getMethods()) {
if (method.isAnnotationPresent(PostConstruct.class)) {
try {
diff --git
a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java
b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java
index 63231cfa14..dae095a193 100644
---
a/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java
+++
b/hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/ConfigurationSource.java
@@ -177,8 +177,7 @@ public interface ConfigurationSource {
.injectConfiguration(this, configurationClass, configObject,
prefix, false);
- ConfigurationReflectionUtil
- .callPostConstruct(configurationClass, configObject);
+ ConfigurationReflectionUtil.callPostConstruct(configObject);
return configObject;
diff --git
a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/ReconfigurationHandler.java
b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/ReconfigurationHandler.java
index 8eae49d083..7a08e47ffa 100644
---
a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/ReconfigurationHandler.java
+++
b/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/conf/ReconfigurationHandler.java
@@ -19,6 +19,7 @@ package org.apache.hadoop.hdds.conf;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.conf.ReconfigurableBase;
+import org.apache.hadoop.conf.ReconfigurationException;
import org.apache.hadoop.conf.ReconfigurationTaskStatus;
import org.apache.hadoop.hdds.protocol.ReconfigureProtocol;
import org.apache.ratis.util.function.CheckedConsumer;
@@ -49,9 +50,9 @@ public class ReconfigurationHandler extends ReconfigurableBase
public ReconfigurationHandler(String name, OzoneConfiguration config,
CheckedConsumer<String, IOException> requireAdminPrivilege) {
+ super(config);
this.name = name;
this.requireAdminPrivilege = requireAdminPrivilege;
- setConf(config);
}
public ReconfigurationHandler register(
@@ -60,6 +61,16 @@ public class ReconfigurationHandler extends
ReconfigurableBase
return this;
}
+ public ReconfigurationHandler register(ReconfigurableConfig config) {
+ config.reconfigurableProperties().forEach(
+ prop -> properties.put(prop, newValue -> {
+ config.reconfigureProperty(prop, newValue);
+ return newValue;
+ })
+ );
+ return this;
+ }
+
@Override
protected Configuration getNewConf() {
return new OzoneConfiguration();
@@ -71,9 +82,15 @@ public class ReconfigurationHandler extends
ReconfigurableBase
}
@Override
- public String reconfigurePropertyImpl(String property, String newValue) {
- return properties.getOrDefault(property, identity())
- .apply(newValue);
+ public String reconfigurePropertyImpl(String property, String newValue)
+ throws ReconfigurationException {
+ final String oldValue = getConf().get(property);
+ try {
+ return properties.getOrDefault(property, identity())
+ .apply(newValue);
+ } catch (Exception e) {
+ throw new ReconfigurationException(property, newValue, oldValue, e);
+ }
}
@Override
diff --git
a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestReconfigurationHandler.java
b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestReconfigurationHandler.java
index 1166eee33d..a7da5763a0 100644
---
a/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestReconfigurationHandler.java
+++
b/hadoop-hdds/framework/src/test/java/org/apache/hadoop/hdds/conf/TestReconfigurationHandler.java
@@ -17,6 +17,7 @@
*/
package org.apache.hadoop.hdds.conf;
+import org.apache.hadoop.conf.ReconfigurationException;
import org.apache.ratis.util.function.CheckedConsumer;
import org.junit.jupiter.api.Test;
@@ -37,42 +38,75 @@ class TestReconfigurationHandler {
private static final String PROP_A = "some.test.property";
private static final String PROP_B = "other.property";
+ private static final String COMPRESSION_ENABLED =
+ "test.scm.client.compression.enabled";
+ private static final String WAIT = "test.scm.client.wait";
+
private static final CheckedConsumer<String, IOException> ACCEPT = any -> {
};
private static final CheckedConsumer<String, IOException> DENY = any -> {
throw new IOException("access denied");
};
+ private final OzoneConfiguration config = new OzoneConfiguration();
+
private final AtomicReference<String> refA =
new AtomicReference<>("oldA");
private final AtomicReference<String> refB =
new AtomicReference<>("oldB");
+ private final SimpleConfiguration object =
+ config.getObject(SimpleConfiguration.class);
private final AtomicReference<CheckedConsumer<String, IOException>>
adminCheck
= new AtomicReference<>(ACCEPT);
- private final ReconfigurationHandler subject = new ReconfigurationHandler(
- "test", new OzoneConfiguration(), op -> adminCheck.get().accept(op))
- .register(PROP_A, refA::getAndSet)
- .register(PROP_B, refB::getAndSet);
+ private final ReconfigurationHandler subject =
+ new ReconfigurationHandler(
+ "test", config, op -> adminCheck.get().accept(op))
+ .register(PROP_A, refA::getAndSet)
+ .register(PROP_B, refB::getAndSet)
+ .register(object);
+
+ private static Stream<String> expectedReconfigurableProperties() {
+ return Stream.of(PROP_A, PROP_B, COMPRESSION_ENABLED, WAIT);
+ }
@Test
void getProperties() {
- assertEquals(Stream.of(PROP_A, PROP_B).collect(toSet()),
+ assertEquals(expectedReconfigurableProperties().collect(toSet()),
subject.getReconfigurableProperties());
}
@Test
void listProperties() throws IOException {
- assertEquals(Stream.of(PROP_A, PROP_B).sorted().collect(toList()),
+ assertEquals(expectedReconfigurableProperties().sorted().collect(toList()),
subject.listReconfigureProperties());
}
@Test
- void callsReconfigurationFunction() {
- subject.reconfigurePropertyImpl(PROP_A, "newA");
- assertEquals("newA", refA.get());
+ void callsReconfigurationFunction() throws ReconfigurationException {
+ final String newA = "newA";
+ subject.reconfigurePropertyImpl(PROP_A, newA);
+ assertEquals(newA, refA.get());
+
+ final String newB = "newB";
+ subject.reconfigurePropertyImpl(PROP_B, newB);
+ assertEquals(newB, refB.get());
- subject.reconfigurePropertyImpl(PROP_B, "newB");
- assertEquals("newB", refB.get());
+ final boolean newCompressionEnabled = !object.isCompressionEnabled();
+ subject.reconfigurePropertyImpl(COMPRESSION_ENABLED,
+ Boolean.toString(newCompressionEnabled));
+ assertEquals(newCompressionEnabled, object.isCompressionEnabled());
+
+ final long newWaitTime = object.getWaitTime() + 5;
+ subject.reconfigurePropertyImpl(WAIT, Long.toString(newWaitTime));
+ assertEquals(newWaitTime, object.getWaitTime());
+ }
+
+ @Test
+ void validatesNewConfiguration() {
+ final long oldWaitTime = object.getWaitTime();
+ assertThrows(ReconfigurationException.class,
+ () -> subject.reconfigurePropertyImpl(WAIT, "0"));
+ assertEquals(oldWaitTime, object.getWaitTime());
}
@Test
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java
index daa9e65467..a42763f0ce 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestOmReconfiguration.java
@@ -19,6 +19,7 @@
package org.apache.hadoop.ozone.reconfig;
import com.google.common.collect.ImmutableSet;
+import org.apache.hadoop.conf.ReconfigurationException;
import org.apache.hadoop.hdds.conf.ReconfigurationHandler;
import org.junit.jupiter.api.Test;
@@ -44,7 +45,7 @@ class TestOmReconfiguration extends ReconfigurationTestBase {
}
@Test
- void adminUsernames() {
+ void adminUsernames() throws ReconfigurationException {
final String newValue = randomAlphabetic(10);
getSubject().reconfigurePropertyImpl(OZONE_ADMINISTRATORS, newValue);
@@ -55,7 +56,7 @@ class TestOmReconfiguration extends ReconfigurationTestBase {
}
@Test
- void readOnlyAdmins() {
+ void readOnlyAdmins() throws ReconfigurationException {
final String newValue = randomAlphabetic(10);
getSubject().reconfigurePropertyImpl(OZONE_READONLY_ADMINISTRATORS,
diff --git
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestScmReconfiguration.java
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestScmReconfiguration.java
index 84045325dc..a407899c22 100644
---
a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestScmReconfiguration.java
+++
b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/reconfig/TestScmReconfiguration.java
@@ -19,6 +19,7 @@
package org.apache.hadoop.ozone.reconfig;
import com.google.common.collect.ImmutableSet;
+import org.apache.hadoop.conf.ReconfigurationException;
import org.apache.hadoop.hdds.conf.ReconfigurationHandler;
import org.junit.jupiter.api.Test;
@@ -43,7 +44,7 @@ class TestScmReconfiguration extends ReconfigurationTestBase {
}
@Test
- void adminUsernames() {
+ void adminUsernames() throws ReconfigurationException {
final String newValue = randomAlphabetic(10);
getSubject().reconfigurePropertyImpl(OZONE_ADMINISTRATORS, newValue);
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
