From d23922c77e3e99c887eb6feabee861d9581e9e03 Mon Sep 17 00:00:00 2001 From: dragonyliu Date: Tue, 27 Dec 2022 10:55:44 +0800 Subject: [PATCH 1/4] Support re-config raft property --- .../org/apache/ratis/conf/RaftProperties.java | 7 + .../org/apache/ratis/conf/Reconfigurable.java | 69 ++ .../ratis/conf/ReconfigurationBase.java | 277 ++++++++ .../ratis/conf/ReconfigurationException.java | 104 +++ .../ratis/conf/ReconfigurationTaskStatus.java | 67 ++ .../ratis/conf/ReconfigurationUtil.java | 70 ++ .../apache/ratis/TestReConfigProperty.java | 611 ++++++++++++++++++ .../grpc/TestReConfigPropertyWithGrpc.java | 26 + 8 files changed, 1231 insertions(+) create mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java create mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java create mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java create mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java create mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java create mode 100644 ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java create mode 100644 ratis-test/src/test/java/org/apache/ratis/grpc/TestReConfigPropertyWithGrpc.java diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java b/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java index b48f79f99c..1b05695814 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java @@ -708,6 +708,13 @@ public void clear() { properties.clear(); } + /** + * @return the key-value properties map + */ + public ConcurrentMap getProperties() { + return properties; + } + @Override public String toString() { return JavaUtils.getClassSimpleName(getClass()) + ":" + size(); diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java b/ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java new file mode 100644 index 0000000000..edce378ae8 --- /dev/null +++ b/ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java @@ -0,0 +1,69 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis.conf; + +import java.util.Collection; + +public interface Reconfigurable { + /** + * Change a raft property on this object to the value specified. + * + * Change a raft property on this object to the value specified + * and return the previous value that the raft property was set to + * (or null if it was not previously set). If newVal is null, set the property + * to its default value; + * + * @param property property name. + * @param newVal new value. + * @throws ReconfigurationException if there was an error applying newVal. + * If the property cannot be changed, throw a + * {@link ReconfigurationException}. + */ + void reconfigureProperty(String property, String newVal) + throws ReconfigurationException; + + /** + * Return whether a given property is changeable at run time. + * + * If isPropertyReconfigurable returns true for a property, + * then changeConf should not throw an exception when changing + * this property. + * @param property property name. + * @return true if property reconfigurable; false if not. + */ + boolean isPropertyReconfigurable(String property); + + /** + * Return all the properties that can be changed at run time. + * @return reconfigurable properties. + */ + Collection getReconfigurableProperties(); + + /** + * Set the raft properties to be used by this object. + * @param prop raft properties to be used + */ + void setConf(RaftProperties prop); + + /** + * Return the raft properties used by this object. + * @return RaftProperties + */ + RaftProperties getConf(); +} diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java new file mode 100644 index 0000000000..d07a7ce6b3 --- /dev/null +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java @@ -0,0 +1,277 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis.conf; + +import org.apache.ratis.thirdparty.com.google.common.collect.Maps; +import org.apache.ratis.util.Daemon; +import org.apache.ratis.util.TimeDuration; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.io.IOException; +import java.sql.Time; +import java.util.Collection; +import java.util.Collections; +import java.util.Map; +import org.apache.ratis.conf.ReconfigurationUtil.PropertyChange; +import java.util.Optional; + +/** + * Utility base class for implementing the Reconfigurable interface. + * + * Subclasses should override reconfigurePropertyImpl to change individual + * properties and getReconfigurableProperties to get all properties that + * can be changed at run time. + */ +public abstract class ReconfigurationBase implements Reconfigurable { + + private static final Logger LOG = LoggerFactory.getLogger(ReconfigurationBase.class); + + private ReconfigurationUtil reconfigurationUtil = new ReconfigurationUtil(); + /** Background thread to reload configuration. */ + private Thread reconfigThread = null; + private volatile boolean shouldRun = true; + private Object reconfigLock = new Object(); + private RaftProperties prop; + /** + * The timestamp when the reconfigThread starts. + */ + private long startTime = 0; + + /** + * The timestamp when the reconfigThread finishes. + */ + private long endTime = 0; + + + /** + * A map of . If error message is present, + * it contains the messages about the error occurred when applies the particular + * change. Otherwise, it indicates that the change has been successfully applied. + */ + private Map> status = null; + + /** + * Construct a ReconfigurableBase. + */ + public ReconfigurationBase() { + setConf(new RaftProperties()); + } + + /** + * Construct a ReconfigurableBase with the {@link RaftProperties} + * @param prop raft properties. + */ + public ReconfigurationBase(RaftProperties prop) { + setConf((prop == null) ? new RaftProperties() : prop); + } + + @Override + public void setConf(RaftProperties prop) { + this.prop = prop; + } + + public RaftProperties getConf() { + return prop; + } + + /** + * Create a new raft properties. + * @return raftProperties. + */ + protected abstract RaftProperties getNewConf(); + + public Collection getChangedProperties( + RaftProperties newConf, RaftProperties oldConf) { + return reconfigurationUtil.parseChangedProperties(newConf, oldConf); + } + + /** + * A background thread to apply property changes. + */ + private static class ReconfigurationThread extends Thread { + private ReconfigurationBase parent; + + ReconfigurationThread(ReconfigurationBase base) { + this.parent = base; + } + + // See {@link ReconfigurationServlet#applyChanges} + public void run() { + LOG.info("Starting reconfiguration task."); + final RaftProperties oldConf = parent.getConf(); + final RaftProperties newConf = parent.getNewConf(); + final Collection changes = + parent.getChangedProperties(newConf, oldConf); + Map> results = Maps.newHashMap(); + for (PropertyChange change : changes) { + String errorMessage = null; + if (!parent.isPropertyReconfigurable(change.prop)) { + LOG.info(String.format( + "Property %s is not configurable: old value: %s, new value: %s", + change.prop, + change.oldVal, + change.newVal)); + continue; + } + LOG.info("Change property: " + change.prop + " from \"" + + ((change.oldVal == null) ? "" : change.oldVal) + + "\" to \"" + + ((change.newVal == null) ? "" : change.newVal) + + "\"."); + try { + String effectiveValue = + parent.reconfigurePropertyImpl(change.prop, change.newVal); + if (change.newVal != null) { + oldConf.set(change.prop, effectiveValue); + } else { + oldConf.unset(change.prop); + } + } catch (ReconfigurationException e) { + Throwable cause = e.getCause(); + errorMessage = cause == null ? e.getMessage() : cause.getMessage(); + } + results.put(change, Optional.ofNullable(errorMessage)); + } + + synchronized (parent.reconfigLock) { + parent.endTime = System.nanoTime(); + parent.status = Collections.unmodifiableMap(results); + parent.reconfigThread = null; + } + } + } + + /** + * Start a reconfiguration task to reload raft property in background. + * @throws IOException raised on errors performing I/O. + */ + public void startReconfigurationTask() throws IOException { + synchronized (reconfigLock) { + if (!shouldRun) { + String errorMessage = "The server is stopped."; + LOG.warn(errorMessage); + throw new IOException(errorMessage); + } + if (reconfigThread != null) { + String errorMessage = "Another reconfiguration task is running."; + LOG.warn(errorMessage); + throw new IOException(errorMessage); + } + reconfigThread = new ReconfigurationThread(this); + reconfigThread.setDaemon(true); + reconfigThread.setName("Reconfiguration Task"); + reconfigThread.start(); + startTime = System.nanoTime(); + } + } + + public ReconfigurationTaskStatus getReconfigurationTaskStatus() { + synchronized (reconfigLock) { + if (reconfigThread != null) { + return new ReconfigurationTaskStatus(startTime, 0, null); + } + return new ReconfigurationTaskStatus(startTime, endTime, status); + } + } + + public void shutdownReconfigurationTask() { + Thread tempThread; + synchronized (reconfigLock) { + shouldRun = false; + if (reconfigThread == null) { + return; + } + tempThread = reconfigThread; + reconfigThread = null; + } + + try { + tempThread.join(); + } catch (InterruptedException e) { + } + } + + /** + * {@inheritDoc} + * + * This method makes the change to this objects {@link RaftProperties} + * and calls reconfigurePropertyImpl to update internal data structures. + * This method cannot be overridden, subclasses should instead override + * reconfigurePropertyImpl. + */ + @Override + public final void reconfigureProperty(String property, String newVal) + throws ReconfigurationException { + if (isPropertyReconfigurable(property)) { + LOG.info("changing property " + property + " to " + newVal); + synchronized(getConf()) { + getConf().get(property); + String effectiveValue = reconfigurePropertyImpl(property, newVal); + if (newVal != null) { + getConf().set(property, effectiveValue); + } else { + getConf().unset(property); + } + } + } else { + throw new ReconfigurationException(property, newVal, + getConf().get(property)); + } + } + + /** + * {@inheritDoc} + * + * Subclasses must override this. + */ + @Override + public abstract Collection getReconfigurableProperties(); + + + /** + * {@inheritDoc} + * + * Subclasses may wish to override this with a more efficient implementation. + */ + @Override + public boolean isPropertyReconfigurable(String property) { + return getReconfigurableProperties().contains(property); + } + + /** + * Change a configuration property. + * + * Subclasses must override this. This method applies the change to + * all internal data structures derived from the configuration property + * that is being changed. If this object owns other Reconfigurable objects + * reconfigureProperty should be called recursively to make sure that + * the configuration of these objects are updated. + * + * @param property Name of the property that is being reconfigured. + * @param newVal Proposed new value of the property. + * @return Effective new value of the property. This may be different from + * newVal. + * + * @throws ReconfigurationException if there was an error applying newVal. + */ + protected abstract String reconfigurePropertyImpl(String property, String newVal) + throws ReconfigurationException; + +} diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java new file mode 100644 index 0000000000..192f2aebc0 --- /dev/null +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java @@ -0,0 +1,104 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis.conf; + +public class ReconfigurationException extends Exception{ + + private static final long serialVersionUID = 1L; + + private String property; + private String newVal; + private String oldVal; + + /** + * Construct the exception message. + */ + private static String constructMessage(String property, String newVal, String oldVal) { + String message = "Could not change property " + property; + if (oldVal != null) { + message += " from \'" + oldVal; + } + if (newVal != null) { + message += "\' to \'" + newVal + "\'"; + } + return message; + } + + + /** + * Create a new instance of {@link ReconfigurationException}. + */ + public ReconfigurationException() { + super("Could not change configuration."); + this.property = null; + this.newVal = null; + this.oldVal = null; + } + + /** + * Create a new instance of {@link ReconfigurationException}. + * @param property property name. + * @param newVal new value. + * @param oldVal old value. + * @param cause original exception. + */ + public ReconfigurationException(String property, String newVal, String oldVal, Throwable cause) { + super(constructMessage(property, newVal, oldVal), cause); + this.property = property; + this.newVal = newVal; + this.oldVal = oldVal; + } + + /** + * Create a new instance of {@link ReconfigurationException}. + * @param property property name. + * @param newVal new value. + * @param oldVal old value. + */ + public ReconfigurationException(String property, String newVal, String oldVal) { + super(constructMessage(property, newVal, oldVal)); + this.property = property; + this.newVal = newVal; + this.oldVal = oldVal; + } + + /** + * Get property that cannot be changed. + * @return property info. + */ + public String getProperty() { + return property; + } + + /** + * Get value to which property was supposed to be changed. + * @return new value. + */ + public String getNewValue() { + return newVal; + } + + /** + * Get old value of property that cannot be changed. + * @return old value. + */ + public String getOldValue() { + return oldVal; + } +} diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java new file mode 100644 index 0000000000..cc8f2523df --- /dev/null +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis.conf; + +import java.util.Map; +import java.util.Optional; +import org.apache.ratis.conf.ReconfigurationUtil.PropertyChange; + +public class ReconfigurationTaskStatus { + long startTime; + long endTime; + final Map> status; + + public ReconfigurationTaskStatus(long startTime, long endTime, + Map> status) { + this.startTime = startTime; + this.endTime = endTime; + this.status = status; + } + + /** + * Return true if + * - A reconfiguration task has finished or + * - an active reconfiguration task is running. + * @return true if startTime > 0; false if not. + */ + public boolean hasTask() { + return startTime > 0; + } + + /** + * Return true if the latest reconfiguration task has finished and there is + * no another active task running. + * @return true if endTime > 0; false if not. + */ + public boolean stopped() { + return endTime > 0; + } + + public long getStartTime() { + return startTime; + } + + public long getEndTime() { + return endTime; + } + + public final Map> getStatus() { + return status; + } +} diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java new file mode 100644 index 0000000000..9fe10acab8 --- /dev/null +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java @@ -0,0 +1,70 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis.conf; + +import java.util.Collection; +import java.util.HashMap; +import java.util.Map; + +public class ReconfigurationUtil { + public static class PropertyChange { + public String prop; + public String oldVal; + public String newVal; + + public PropertyChange(String prop, String newVal, String oldVal) { + this.prop = prop; + this.newVal = newVal; + this.oldVal = oldVal; + } + } + + public static Collection + getChangedProperties(RaftProperties newConf, RaftProperties oldConf) { + Map changes = new HashMap(); + + // iterate over old configuration + for (Map.Entry oldEntry: oldConf.getProperties().entrySet()) { + String prop = oldEntry.getKey(); + String oldVal = oldEntry.getValue(); + String newVal = newConf.getRaw(prop); + + if (newVal == null || !newVal.equals(oldVal)) { + changes.put(prop, new PropertyChange(prop, newVal, oldVal)); + } + } + + // now iterate over new configuration + // (to look for properties not present in old conf) + for (Map.Entry newEntry: newConf.getProperties().entrySet()) { + String prop = newEntry.getKey(); + String newVal = newEntry.getValue(); + if (oldConf.get(prop) == null) { + changes.put(prop, new PropertyChange(prop, newVal, null)); + } + } + + return changes.values(); + } + + public Collection parseChangedProperties( + RaftProperties newConf, RaftProperties oldConf) { + return getChangedProperties(newConf, oldConf); + } +} diff --git a/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java b/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java new file mode 100644 index 0000000000..6e20878924 --- /dev/null +++ b/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java @@ -0,0 +1,611 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis; + +import static org.junit.Assert.fail; +import static org.junit.matchers.JUnitMatchers.containsString; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.spy; + +import org.apache.log4j.Level; +import org.apache.ratis.client.impl.OrderedAsync; +import org.apache.ratis.conf.RaftProperties; +import org.apache.ratis.conf.ReconfigurationBase; +import org.apache.ratis.conf.ReconfigurationException; +import org.apache.ratis.conf.ReconfigurationTaskStatus; +import org.apache.ratis.conf.ReconfigurationUtil; +import org.apache.ratis.conf.ReconfigurationUtil.PropertyChange; +import org.apache.ratis.server.impl.MiniRaftCluster; +import org.apache.ratis.statemachine.StateMachine; +import org.apache.ratis.statemachine.impl.SimpleStateMachine4Testing; +import org.apache.ratis.thirdparty.com.google.common.collect.Lists; +import org.apache.ratis.util.Log4jUtils; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeoutException; + +public abstract class TestReConfigProperty extends BaseTest + implements MiniRaftCluster.Factory.Get { + + { + Log4jUtils.setLogLevel(OrderedAsync.LOG, Level.DEBUG); + getProperties().setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY, + SimpleStateMachine4Testing.class, StateMachine.class); + } + + private RaftProperties conf1; + private RaftProperties conf2; + + private static final String PROP1 = "test.prop.one"; + private static final String PROP2 = "test.prop.two"; + private static final String PROP3 = "test.prop.three"; + private static final String PROP4 = "test.prop.four"; + private static final String PROP5 = "test.prop.five"; + + private static final String VAL1 = "val1"; + private static final String VAL2 = "val2"; + + + @Before + public void setup () { + conf1 = new RaftProperties(); + conf2 = new RaftProperties(); + + // set some test properties + conf1.set(PROP1, VAL1); + conf1.set(PROP2, VAL1); + conf1.set(PROP3, VAL1); + + conf2.set(PROP1, VAL1); // same as conf1 + conf2.set(PROP2, VAL2); // different value as conf1 + // PROP3 not set in conf2 + conf2.set(PROP4, VAL1); // not set in conf1 + + } + + @Test + public void testGetChangedProperty() { + Collection changes + = ReconfigurationUtil.getChangedProperties(conf2, conf1); + + Assert.assertTrue("expected 3 changed properties but got " + changes.size(), + changes.size() == 3); + + boolean changeFound = false; + boolean unsetFound = false; + boolean setFound = false; + + for (ReconfigurationUtil.PropertyChange c: changes) { + if (c.prop.equals(PROP2) && c.oldVal != null && c.oldVal.equals(VAL1) && + c.newVal != null && c.newVal.equals(VAL2)) { + changeFound = true; + } else if (c.prop.equals(PROP3) && c.oldVal != null && c.oldVal.equals(VAL1) && + c.newVal == null) { + unsetFound = true; + } else if (c.prop.equals(PROP4) && c.oldVal == null && + c.newVal != null && c.newVal.equals(VAL1)) { + setFound = true; + } + } + Assert.assertTrue("not all changes have been applied", + changeFound && unsetFound && setFound); + } + + /** + * a simple reconfigurable class + */ + public static class ReconfigurableDummy extends ReconfigurationBase + implements Runnable { + public volatile boolean running = true; + + public ReconfigurableDummy(RaftProperties prop) { + super(prop); + } + + @Override + protected RaftProperties getNewConf() { + return new RaftProperties(); + } + + @Override + public Collection getReconfigurableProperties() { + return Arrays.asList(PROP1, PROP2, PROP4); + } + + @Override + public synchronized String reconfigurePropertyImpl( + String property, String newVal) throws ReconfigurationException { + // do nothing + return newVal; + } + + /** + * Run until PROP1 is no longer VAL1. + */ + @Override + public void run() { + while (running && getConf().get(PROP1).equals(VAL1)) { + try { + Thread.sleep(1); + } catch (InterruptedException ignore) { + // do nothing + } + } + } + + } + + /** + * Test reconfiguring a Reconfigurable. + */ + @Test + public void testReconfigure() { + ReconfigurableDummy dummy = new ReconfigurableDummy(conf1); + + Assert.assertTrue(PROP1 + " set to wrong value ", + dummy.getConf().get(PROP1).equals(VAL1)); + Assert.assertTrue(PROP2 + " set to wrong value ", + dummy.getConf().get(PROP2).equals(VAL1)); + Assert.assertTrue(PROP3 + " set to wrong value ", + dummy.getConf().get(PROP3).equals(VAL1)); + Assert.assertTrue(PROP4 + " set to wrong value ", + dummy.getConf().get(PROP4) == null); + Assert.assertTrue(PROP5 + " set to wrong value ", + dummy.getConf().get(PROP5) == null); + + Assert.assertTrue(PROP1 + " should be reconfigurable ", + dummy.isPropertyReconfigurable(PROP1)); + Assert.assertTrue(PROP2 + " should be reconfigurable ", + dummy.isPropertyReconfigurable(PROP2)); + Assert.assertFalse(PROP3 + " should not be reconfigurable ", + dummy.isPropertyReconfigurable(PROP3)); + Assert.assertTrue(PROP4 + " should be reconfigurable ", + dummy.isPropertyReconfigurable(PROP4)); + Assert.assertFalse(PROP5 + " should not be reconfigurable ", + dummy.isPropertyReconfigurable(PROP5)); + + // change something to the same value as before + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP1, VAL1); + Assert.assertTrue(PROP1 + " set to wrong value ", + dummy.getConf().get(PROP1).equals(VAL1)); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertFalse("received unexpected exception", + exceptionCaught); + } + + // change something to null + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP1, null); + Assert.assertTrue(PROP1 + "set to wrong value ", + dummy.getConf().get(PROP1) == null); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertFalse("received unexpected exception", + exceptionCaught); + } + + // change something to a different value than before + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP1, VAL2); + Assert.assertTrue(PROP1 + "set to wrong value ", + dummy.getConf().get(PROP1).equals(VAL2)); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertFalse("received unexpected exception", + exceptionCaught); + } + + // set unset property to null + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP4, null); + Assert.assertTrue(PROP4 + "set to wrong value ", + dummy.getConf().get(PROP4) == null); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertFalse("received unexpected exception", + exceptionCaught); + } + + // set unset property + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP4, VAL1); + Assert.assertTrue(PROP4 + "set to wrong value ", + dummy.getConf().get(PROP4).equals(VAL1)); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertFalse("received unexpected exception", + exceptionCaught); + } + + // try to set unset property to null (not reconfigurable) + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP5, null); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertTrue("did not receive expected exception", + exceptionCaught); + } + + // try to set unset property to value (not reconfigurable) + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP5, VAL1); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertTrue("did not receive expected exception", + exceptionCaught); + } + + // try to change property to value (not reconfigurable) + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP3, VAL2); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertTrue("did not receive expected exception", + exceptionCaught); + } + + // try to change property to null (not reconfigurable) + { + boolean exceptionCaught = false; + try { + dummy.reconfigureProperty(PROP3, null); + } catch (ReconfigurationException e) { + exceptionCaught = true; + } + Assert.assertTrue("did not receive expected exception", + exceptionCaught); + } + } + + /** + * Test whether configuration changes are visible in another thread. + */ + @Test + public void testThread() throws ReconfigurationException { + ReconfigurableDummy dummy = new ReconfigurableDummy(conf1); + Assert.assertTrue(dummy.getConf().get(PROP1).equals(VAL1)); + Thread dummyThread = new Thread(dummy); + dummyThread.start(); + try { + Thread.sleep(500); + } catch (InterruptedException ignore) { + // do nothing + } + dummy.reconfigureProperty(PROP1, VAL2); + + long endWait = System.currentTimeMillis() + 2000; + while (dummyThread.isAlive() && System.currentTimeMillis() < endWait) { + try { + Thread.sleep(50); + } catch (InterruptedException ignore) { + // do nothing + } + } + + Assert.assertFalse("dummy thread should not be alive", + dummyThread.isAlive()); + dummy.running = false; + try { + dummyThread.join(); + } catch (InterruptedException ignore) { + // do nothing + } + Assert.assertTrue(PROP1 + " is set to wrong value", + dummy.getConf().get(PROP1).equals(VAL2)); + + } + + private static class AsyncReconfigurableDummy extends ReconfigurationBase { + + AsyncReconfigurableDummy(RaftProperties prop) { + super(prop); + } + + @Override + protected RaftProperties getNewConf() { + return new RaftProperties(); + } + + final CountDownLatch latch = new CountDownLatch(1); + + @Override + public Collection getReconfigurableProperties() { + return Arrays.asList(PROP1, PROP2, PROP4); + } + + @Override + public synchronized String reconfigurePropertyImpl(String property, + String newVal) throws ReconfigurationException { + try { + latch.await(); + } catch (InterruptedException e) { + // Ignore + } + return newVal; + } + } + + private static void waitAsyncReconfigureTaskFinish(ReconfigurationBase rb) + throws InterruptedException { + ReconfigurationTaskStatus status = null; + int count = 20; + while (count > 0) { + status = rb.getReconfigurationTaskStatus(); + if (status.stopped()) { + break; + } + count--; + Thread.sleep(500); + } + assert(status.stopped()); + } + + @Test + public void testAsyncReconfigure() + throws ReconfigurationException, IOException, InterruptedException { + AsyncReconfigurableDummy dummy = spy(new AsyncReconfigurableDummy(conf1)); + + List changes = Lists.newArrayList(); + changes.add(new PropertyChange("name1", "new1", "old1")); + changes.add(new PropertyChange("name2", "new2", "old2")); + changes.add(new PropertyChange("name3", "new3", "old3")); + doReturn(changes).when(dummy).getChangedProperties( + any(RaftProperties.class), any(RaftProperties.class)); + + doReturn(true).when(dummy).isPropertyReconfigurable(eq("name1")); + doReturn(false).when(dummy).isPropertyReconfigurable(eq("name2")); + doReturn(true).when(dummy).isPropertyReconfigurable(eq("name3")); + + doReturn("dummy").when(dummy) + .reconfigurePropertyImpl(eq("name1"), anyString()); + doReturn("dummy").when(dummy) + .reconfigurePropertyImpl(eq("name2"), anyString()); + doThrow(new ReconfigurationException("NAME3", "NEW3", "OLD3", + new IOException("io exception"))) + .when(dummy).reconfigurePropertyImpl(eq("name3"), anyString()); + + dummy.startReconfigurationTask(); + + waitAsyncReconfigureTaskFinish(dummy); + ReconfigurationTaskStatus status = dummy.getReconfigurationTaskStatus(); + Assert.assertEquals(2, status.getStatus().size()); + for (Map.Entry> result : + status.getStatus().entrySet()) { + PropertyChange change = result.getKey(); + if (change.prop.equals("name1")) { + Assert.assertFalse(result.getValue().isPresent()); + } else if (change.prop.equals("name2")) { + Assert.assertThat(result.getValue().get(), + containsString("Property name2 is not reconfigurable")); + } else if (change.prop.equals("name3")) { + Assert.assertThat(result.getValue().get(), containsString("io exception")); + } else { + fail("Unknown property: " + change.prop); + } + } + } + + @Test(timeout=30000) + public void testStartReconfigurationFailureDueToExistingRunningTask() + throws InterruptedException, IOException { + AsyncReconfigurableDummy dummy = spy(new AsyncReconfigurableDummy(conf1)); + List changes = Lists.newArrayList( + new PropertyChange(PROP1, "new1", "old1") + ); + doReturn(changes).when(dummy).getChangedProperties( + any(RaftProperties.class), any(RaftProperties.class)); + + ReconfigurationTaskStatus status = dummy.getReconfigurationTaskStatus(); + Assert.assertFalse(status.hasTask()); + + dummy.startReconfigurationTask(); + status = dummy.getReconfigurationTaskStatus(); + Assert.assertTrue(status.hasTask()); + Assert.assertFalse(status.stopped()); + + // An active reconfiguration task is running. + try { + dummy.startReconfigurationTask(); + fail("Expect to throw IOException."); + } catch (IOException e) { + Assert.assertTrue(e.getMessage().contains("Another reconfiguration task is running")); + } + status = dummy.getReconfigurationTaskStatus(); + Assert.assertTrue(status.hasTask()); + Assert.assertFalse(status.stopped()); + + dummy.latch.countDown(); + waitAsyncReconfigureTaskFinish(dummy); + status = dummy.getReconfigurationTaskStatus(); + Assert.assertTrue(status.hasTask()); + Assert.assertTrue(status.stopped()); + + // The first task has finished. + dummy.startReconfigurationTask(); + waitAsyncReconfigureTaskFinish(dummy); + ReconfigurationTaskStatus status2 = dummy.getReconfigurationTaskStatus(); + Assert.assertTrue(status2.getStartTime() >= status.getEndTime()); + + dummy.shutdownReconfigurationTask(); + try { + dummy.startReconfigurationTask(); + fail("Expect to throw IOException"); + } catch (IOException e) { + Assert.assertTrue(e.getMessage().contains("The server is stopped")); + } + } + + /** + * Ensure that {@link ReconfigurationBase#reconfigureProperty} updates the + * parent's cached configuration on success. + * @throws IOException + */ + @Test (timeout=300000) + public void testConfIsUpdatedOnSuccess() throws ReconfigurationException { + final String property = "FOO"; + final String value1 = "value1"; + final String value2 = "value2"; + + final RaftProperties conf = new RaftProperties(); + conf.set(property, value1); + final RaftProperties newConf = new RaftProperties(); + newConf.set(property, value2); + + final ReconfigurationBase reconfigurable = makeReconfigurable( + conf, newConf, Arrays.asList(property)); + + reconfigurable.reconfigureProperty(property, value2); + Assert.assertEquals(value2, reconfigurable.getConf().get(property)); + } + + /** + * Ensure that {@link ReconfigurationBase#startReconfigurationTask} updates + * its parent's cached configuration on success. + * @throws IOException + */ + @Test (timeout=300000) + public void testConfIsUpdatedOnSuccessAsync() + throws InterruptedException, IOException, TimeoutException { + final String property = "FOO"; + final String value1 = "value1"; + final String value2 = "value2"; + + final RaftProperties conf = new RaftProperties(); + conf.set(property, value1); + final RaftProperties newConf = new RaftProperties(); + newConf.set(property, value2); + + final ReconfigurationBase reconfigurable = makeReconfigurable( + conf, newConf, Arrays.asList(property)); + + // Kick off a reconfiguration task and wait until it completes. + reconfigurable.startReconfigurationTask(); + + RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationTaskStatus().stopped(), 100, 60000); + Assert.assertEquals(value2, reconfigurable.getConf().get(property)); + } + + /** + * Ensure that {@link ReconfigurationBase#reconfigureProperty} unsets the + * property in its parent's configuration when the new value is null. + * @throws IOException + */ + @Test (timeout=300000) + public void testConfIsUnset() throws ReconfigurationException { + final String property = "FOO"; + final String value1 = "value1"; + + final RaftProperties conf = new RaftProperties(); + conf.set(property, value1); + final RaftProperties newConf = new RaftProperties(); + + final ReconfigurationBase reconfigurable = makeReconfigurable( + conf, newConf, Arrays.asList(property)); + + reconfigurable.reconfigureProperty(property, null); + Assert.assertNull(reconfigurable.getConf().get(property)); + } + + /** + * Ensure that {@link ReconfigurationBase#startReconfigurationTask} unsets the + * property in its parent's configuration when the new value is null. + * @throws IOException + */ + @Test (timeout=300000) + public void testConfIsUnsetAsync() throws ReconfigurationException, + IOException, TimeoutException, InterruptedException { + final String property = "FOO"; + final String value1 = "value1"; + + final RaftProperties conf = new RaftProperties(); + conf.set(property, value1); + final RaftProperties newConf = new RaftProperties(); + + final ReconfigurationBase reconfigurable = makeReconfigurable( + conf, newConf, Arrays.asList(property)); + + // Kick off a reconfiguration task and wait until it completes. + reconfigurable.startReconfigurationTask(); + RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationTaskStatus().stopped(), 100, 60000); + Assert.assertNull(reconfigurable.getConf().get(property)); + } + + private ReconfigurationBase makeReconfigurable( + final RaftProperties oldConf, final RaftProperties newConf, + final Collection reconfigurableProperties) { + final RaftProperties prop; + + return new ReconfigurationBase(oldConf) { + @Override + protected RaftProperties getNewConf() { + return newConf; + } + + @Override + public Collection getReconfigurableProperties() { + return reconfigurableProperties; + } + + @Override + protected String reconfigurePropertyImpl( + String property, String newVal) throws ReconfigurationException { + return newVal; + } + }; + } +} diff --git a/ratis-test/src/test/java/org/apache/ratis/grpc/TestReConfigPropertyWithGrpc.java b/ratis-test/src/test/java/org/apache/ratis/grpc/TestReConfigPropertyWithGrpc.java new file mode 100644 index 0000000000..a57fb86ad5 --- /dev/null +++ b/ratis-test/src/test/java/org/apache/ratis/grpc/TestReConfigPropertyWithGrpc.java @@ -0,0 +1,26 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis.grpc; + + +import org.apache.ratis.TestReConfigProperty; + +public class TestReConfigPropertyWithGrpc extends TestReConfigProperty + implements MiniRaftClusterWithGrpc.FactoryGet{ +} From 6f06fb181192d086946889f4f389c1243cfd9715 Mon Sep 17 00:00:00 2001 From: dragonyliu Date: Tue, 27 Dec 2022 11:37:24 +0800 Subject: [PATCH 2/4] fix checkstyle and findbugs --- .../ratis/conf/ReconfigurationBase.java | 35 +++++++++---------- .../ratis/conf/ReconfigurationTaskStatus.java | 6 ++-- .../ratis/conf/ReconfigurationUtil.java | 18 ++++++++-- .../apache/ratis/TestReConfigProperty.java | 26 +++++++------- 4 files changed, 47 insertions(+), 38 deletions(-) diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java index d07a7ce6b3..445c2d8c12 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java @@ -19,13 +19,10 @@ package org.apache.ratis.conf; import org.apache.ratis.thirdparty.com.google.common.collect.Maps; -import org.apache.ratis.util.Daemon; -import org.apache.ratis.util.TimeDuration; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; -import java.sql.Time; import java.util.Collection; import java.util.Collections; import java.util.Map; @@ -76,15 +73,15 @@ public ReconfigurationBase() { /** * Construct a ReconfigurableBase with the {@link RaftProperties} - * @param prop raft properties. + * @param properties raft properties. */ - public ReconfigurationBase(RaftProperties prop) { - setConf((prop == null) ? new RaftProperties() : prop); + public ReconfigurationBase(RaftProperties properties) { + setConf((properties == null) ? new RaftProperties() : properties); } @Override - public void setConf(RaftProperties prop) { - this.prop = prop; + public void setConf(RaftProperties properties) { + this.prop = properties; } public RaftProperties getConf() { @@ -122,26 +119,26 @@ public void run() { Map> results = Maps.newHashMap(); for (PropertyChange change : changes) { String errorMessage = null; - if (!parent.isPropertyReconfigurable(change.prop)) { + if (!parent.isPropertyReconfigurable(change.getProp())) { LOG.info(String.format( "Property %s is not configurable: old value: %s, new value: %s", - change.prop, - change.oldVal, - change.newVal)); + change.getProp(), + change.getOldVal(), + change.getNewVal())); continue; } - LOG.info("Change property: " + change.prop + " from \"" - + ((change.oldVal == null) ? "" : change.oldVal) + LOG.info("Change property: " + change.getProp() + " from \"" + + ((change.getOldVal() == null) ? "" : change.getOldVal()) + "\" to \"" - + ((change.newVal == null) ? "" : change.newVal) + + ((change.getNewVal() == null) ? "" : change.getNewVal()) + "\"."); try { String effectiveValue = - parent.reconfigurePropertyImpl(change.prop, change.newVal); - if (change.newVal != null) { - oldConf.set(change.prop, effectiveValue); + parent.reconfigurePropertyImpl(change.getProp(), change.getNewVal()); + if (change.getNewVal() != null) { + oldConf.set(change.getProp(), effectiveValue); } else { - oldConf.unset(change.prop); + oldConf.unset(change.getProp()); } } catch (ReconfigurationException e) { Throwable cause = e.getCause(); diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java index cc8f2523df..7efc2d9dbf 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java @@ -23,9 +23,9 @@ import org.apache.ratis.conf.ReconfigurationUtil.PropertyChange; public class ReconfigurationTaskStatus { - long startTime; - long endTime; - final Map> status; + private long startTime; + private long endTime; + private final Map> status; public ReconfigurationTaskStatus(long startTime, long endTime, Map> status) { diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java index 9fe10acab8..a2f0ee92e3 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java @@ -24,15 +24,27 @@ public class ReconfigurationUtil { public static class PropertyChange { - public String prop; - public String oldVal; - public String newVal; + private final String prop; + private final String oldVal; + private final String newVal; public PropertyChange(String prop, String newVal, String oldVal) { this.prop = prop; this.newVal = newVal; this.oldVal = oldVal; } + + public String getProp() { + return prop; + } + + public String getOldVal() { + return oldVal; + } + + public String getNewVal() { + return newVal; + } } public static Collection diff --git a/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java b/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java index 6e20878924..587c785f52 100644 --- a/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java +++ b/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java @@ -27,7 +27,6 @@ import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.spy; -import org.apache.log4j.Level; import org.apache.ratis.client.impl.OrderedAsync; import org.apache.ratis.conf.RaftProperties; import org.apache.ratis.conf.ReconfigurationBase; @@ -39,10 +38,11 @@ import org.apache.ratis.statemachine.StateMachine; import org.apache.ratis.statemachine.impl.SimpleStateMachine4Testing; import org.apache.ratis.thirdparty.com.google.common.collect.Lists; -import org.apache.ratis.util.Log4jUtils; +import org.apache.ratis.util.Slf4jUtils; import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.slf4j.event.Level; import java.io.IOException; import java.util.Arrays; @@ -57,7 +57,7 @@ public abstract class TestReConfigProperty exte implements MiniRaftCluster.Factory.Get { { - Log4jUtils.setLogLevel(OrderedAsync.LOG, Level.DEBUG); + Slf4jUtils.setLogLevel(OrderedAsync.LOG, Level.DEBUG); getProperties().setClass(MiniRaftCluster.STATEMACHINE_CLASS_KEY, SimpleStateMachine4Testing.class, StateMachine.class); } @@ -105,14 +105,14 @@ public void testGetChangedProperty() { boolean setFound = false; for (ReconfigurationUtil.PropertyChange c: changes) { - if (c.prop.equals(PROP2) && c.oldVal != null && c.oldVal.equals(VAL1) && - c.newVal != null && c.newVal.equals(VAL2)) { + if (c.getProp().equals(PROP2) && c.getOldVal() != null && c.getOldVal().equals(VAL1) && + c.getNewVal() != null && c.getNewVal().equals(VAL2)) { changeFound = true; - } else if (c.prop.equals(PROP3) && c.oldVal != null && c.oldVal.equals(VAL1) && - c.newVal == null) { + } else if (c.getProp().equals(PROP3) && c.getOldVal() != null && c.getOldVal().equals(VAL1) && + c.getNewVal() == null) { unsetFound = true; - } else if (c.prop.equals(PROP4) && c.oldVal == null && - c.newVal != null && c.newVal.equals(VAL1)) { + } else if (c.getProp().equals(PROP4) && c.getOldVal() == null && + c.getNewVal() != null && c.getNewVal().equals(VAL1)) { setFound = true; } } @@ -427,15 +427,15 @@ public void testAsyncReconfigure() for (Map.Entry> result : status.getStatus().entrySet()) { PropertyChange change = result.getKey(); - if (change.prop.equals("name1")) { + if (change.getProp().equals("name1")) { Assert.assertFalse(result.getValue().isPresent()); - } else if (change.prop.equals("name2")) { + } else if (change.getProp().equals("name2")) { Assert.assertThat(result.getValue().get(), containsString("Property name2 is not reconfigurable")); - } else if (change.prop.equals("name3")) { + } else if (change.getProp().equals("name3")) { Assert.assertThat(result.getValue().get(), containsString("io exception")); } else { - fail("Unknown property: " + change.prop); + fail("Unknown property: " + change.getProp()); } } } From 2da3a05c23376733f8ffd717c2aec66936c740d7 Mon Sep 17 00:00:00 2001 From: dragonyliu Date: Fri, 30 Dec 2022 19:09:35 +0800 Subject: [PATCH 3/4] refactor code and fix ut --- .../org/apache/ratis/conf/RaftProperties.java | 11 +- .../org/apache/ratis/conf/Reconfigurable.java | 62 ++-- .../ratis/conf/ReconfigurationBase.java | 322 ++++++----------- .../ratis/conf/ReconfigurationException.java | 83 ++--- .../ratis/conf/ReconfigurationStatus.java | 151 ++++++++ .../ratis/conf/ReconfigurationTaskStatus.java | 67 ---- .../ratis/conf/ReconfigurationUtil.java | 82 ----- .../apache/ratis/TestReConfigProperty.java | 339 ++++++------------ 8 files changed, 420 insertions(+), 697 deletions(-) create mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationStatus.java delete mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java delete mode 100644 ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java b/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java index 1b05695814..f51bc731f7 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/RaftProperties.java @@ -1,4 +1,4 @@ -/** +/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information @@ -31,6 +31,7 @@ import java.util.Arrays; import java.util.HashSet; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -708,11 +709,9 @@ public void clear() { properties.clear(); } - /** - * @return the key-value properties map - */ - public ConcurrentMap getProperties() { - return properties; + /** @return the property entry set. */ + Set> entrySet() { + return properties.entrySet(); } @Override diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java b/ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java index edce378ae8..6e8a527da5 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/Reconfigurable.java @@ -20,50 +20,40 @@ import java.util.Collection; +/** + * To reconfigure {@link RaftProperties} in runtime. + */ public interface Reconfigurable { + /** @return the {@link RaftProperties} to be reconfigured. */ + RaftProperties getProperties(); + /** - * Change a raft property on this object to the value specified. - * - * Change a raft property on this object to the value specified - * and return the previous value that the raft property was set to - * (or null if it was not previously set). If newVal is null, set the property - * to its default value; + * Change a property on this object to the new value specified. + * If the new value specified is null, reset the property to its default value. + *

+ * This method must apply the change to all internal data structures derived + * from the configuration property that is being changed. + * If this object owns other {@link Reconfigurable} objects, + * it must call this method recursively in order to update all these objects. * - * @param property property name. - * @param newVal new value. - * @throws ReconfigurationException if there was an error applying newVal. - * If the property cannot be changed, throw a - * {@link ReconfigurationException}. + * @param property the name of the given property. + * @param newValue the new value. + * @return the effective value, which could possibly be different from specified new value, + * of the property after reconfiguration. + * @throws ReconfigurationException if the property is not reconfigurable or there is an error applying the new value. */ - void reconfigureProperty(String property, String newVal) - throws ReconfigurationException; + String reconfigureProperty(String property, String newValue) throws ReconfigurationException; /** - * Return whether a given property is changeable at run time. + * Is the given property reconfigurable at runtime? * - * If isPropertyReconfigurable returns true for a property, - * then changeConf should not throw an exception when changing - * this property. - * @param property property name. - * @return true if property reconfigurable; false if not. + * @param property the name of the given property. + * @return true iff the given property is reconfigurable. */ - boolean isPropertyReconfigurable(String property); + default boolean isPropertyReconfigurable(String property) { + return getReconfigurableProperties().contains(property); + } - /** - * Return all the properties that can be changed at run time. - * @return reconfigurable properties. - */ + /** @return all the properties that are reconfigurable at runtime. */ Collection getReconfigurableProperties(); - - /** - * Set the raft properties to be used by this object. - * @param prop raft properties to be used - */ - void setConf(RaftProperties prop); - - /** - * Return the raft properties used by this object. - * @return RaftProperties - */ - RaftProperties getConf(); } diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java index 445c2d8c12..7b0f2c8a20 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java @@ -18,257 +18,163 @@ package org.apache.ratis.conf; -import org.apache.ratis.thirdparty.com.google.common.collect.Maps; +import org.apache.ratis.conf.ReconfigurationStatus.PropertyChange; +import org.apache.ratis.util.Daemon; +import org.apache.ratis.util.Timestamp; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import java.io.IOException; import java.util.Collection; -import java.util.Collections; +import java.util.HashMap; import java.util.Map; -import org.apache.ratis.conf.ReconfigurationUtil.PropertyChange; -import java.util.Optional; +import java.util.Objects; /** - * Utility base class for implementing the Reconfigurable interface. - * - * Subclasses should override reconfigurePropertyImpl to change individual - * properties and getReconfigurableProperties to get all properties that - * can be changed at run time. + * Base class for implementing the {@link Reconfigurable} interface. + * Subclasses must override + * (1) {@link #getReconfigurableProperties()} to return all properties that can be reconfigurable at runtime, + * (2) {@link #getNewProperties()} to return the new {@link RaftProperties} to be reconfigured to, and + * (3) {@link #reconfigureProperty(String, String)} to change individual properties. */ public abstract class ReconfigurationBase implements Reconfigurable { - private static final Logger LOG = LoggerFactory.getLogger(ReconfigurationBase.class); - private ReconfigurationUtil reconfigurationUtil = new ReconfigurationUtil(); - /** Background thread to reload configuration. */ - private Thread reconfigThread = null; - private volatile boolean shouldRun = true; - private Object reconfigLock = new Object(); - private RaftProperties prop; - /** - * The timestamp when the reconfigThread starts. - */ - private long startTime = 0; + public static Collection getChangedProperties(RaftProperties newProperties, RaftProperties oldProperties) { + final Map changes = new HashMap<>(); - /** - * The timestamp when the reconfigThread finishes. - */ - private long endTime = 0; + // iterate over old properties + for (Map.Entry oldEntry: oldProperties.entrySet()) { + final String prop = oldEntry.getKey(); + final String oldVal = oldEntry.getValue(); + final String newVal = newProperties.getRaw(prop); + if (!Objects.equals(newVal, oldVal)) { + changes.put(prop, new PropertyChange(prop, newVal, oldVal)); + } + } - /** - * A map of . If error message is present, - * it contains the messages about the error occurred when applies the particular - * change. Otherwise, it indicates that the change has been successfully applied. - */ - private Map> status = null; + // now iterate over new properties in order to look for properties not present in old properties + for (Map.Entry newEntry: newProperties.entrySet()) { + final String prop = newEntry.getKey(); + final String newVal = newEntry.getValue(); + if (newVal != null && oldProperties.get(prop) == null) { + changes.put(prop, new PropertyChange(prop, newVal, null)); + } + } - /** - * Construct a ReconfigurableBase. - */ - public ReconfigurationBase() { - setConf(new RaftProperties()); + return changes.values(); } - /** - * Construct a ReconfigurableBase with the {@link RaftProperties} - * @param properties raft properties. - */ - public ReconfigurationBase(RaftProperties properties) { - setConf((properties == null) ? new RaftProperties() : properties); - } + class Context { + /** The current reconfiguration status. */ + private ReconfigurationStatus status = new ReconfigurationStatus(null, null, null, null); + /** Is this context stopped? */ + private boolean isStopped; - @Override - public void setConf(RaftProperties properties) { - this.prop = properties; - } + synchronized ReconfigurationStatus getStatus() { + return status; + } - public RaftProperties getConf() { - return prop; - } + synchronized void start() { + if (isStopped) { + throw new IllegalStateException(name + " is stopped."); + } + final Daemon previous = status.getDaemon(); + if (previous != null) { + throw new IllegalStateException(name + ": a reconfiguration task " + previous + " is already running."); + } + final Timestamp startTime = Timestamp.currentTime(); + final Daemon task = Daemon.newBuilder() + .setName("started@" + startTime) + .setRunnable(ReconfigurationBase.this::batchReconfiguration) + .build(); + status = new ReconfigurationStatus(startTime, null, null, task); + task.start(); + } - /** - * Create a new raft properties. - * @return raftProperties. - */ - protected abstract RaftProperties getNewConf(); + synchronized void end(Map results) { + status = new ReconfigurationStatus(status.getStartTime(), Timestamp.currentTime(), results, null); + } - public Collection getChangedProperties( - RaftProperties newConf, RaftProperties oldConf) { - return reconfigurationUtil.parseChangedProperties(newConf, oldConf); + synchronized Daemon stop() { + isStopped = true; + final Daemon task = status.getDaemon(); + status = new ReconfigurationStatus(status.getStartTime(), null, null, null); + return task; + } } + private final String name; + private final RaftProperties properties; + private final Context context; + /** - * A background thread to apply property changes. + * Construct a ReconfigurableBase with the {@link RaftProperties} + * @param properties raft properties. */ - private static class ReconfigurationThread extends Thread { - private ReconfigurationBase parent; - - ReconfigurationThread(ReconfigurationBase base) { - this.parent = base; - } - - // See {@link ReconfigurationServlet#applyChanges} - public void run() { - LOG.info("Starting reconfiguration task."); - final RaftProperties oldConf = parent.getConf(); - final RaftProperties newConf = parent.getNewConf(); - final Collection changes = - parent.getChangedProperties(newConf, oldConf); - Map> results = Maps.newHashMap(); - for (PropertyChange change : changes) { - String errorMessage = null; - if (!parent.isPropertyReconfigurable(change.getProp())) { - LOG.info(String.format( - "Property %s is not configurable: old value: %s, new value: %s", - change.getProp(), - change.getOldVal(), - change.getNewVal())); - continue; - } - LOG.info("Change property: " + change.getProp() + " from \"" - + ((change.getOldVal() == null) ? "" : change.getOldVal()) - + "\" to \"" - + ((change.getNewVal() == null) ? "" : change.getNewVal()) - + "\"."); - try { - String effectiveValue = - parent.reconfigurePropertyImpl(change.getProp(), change.getNewVal()); - if (change.getNewVal() != null) { - oldConf.set(change.getProp(), effectiveValue); - } else { - oldConf.unset(change.getProp()); - } - } catch (ReconfigurationException e) { - Throwable cause = e.getCause(); - errorMessage = cause == null ? e.getMessage() : cause.getMessage(); - } - results.put(change, Optional.ofNullable(errorMessage)); - } + public ReconfigurationBase(String name, RaftProperties properties) { + this.name = name; + this.properties = properties; + this.context = new Context(); + } - synchronized (parent.reconfigLock) { - parent.endTime = System.nanoTime(); - parent.status = Collections.unmodifiableMap(results); - parent.reconfigThread = null; - } - } + @Override + public RaftProperties getProperties() { + return properties; } + /** @return the new {@link RaftProperties} to be reconfigured to. */ + protected abstract RaftProperties getNewProperties(); + /** * Start a reconfiguration task to reload raft property in background. * @throws IOException raised on errors performing I/O. */ - public void startReconfigurationTask() throws IOException { - synchronized (reconfigLock) { - if (!shouldRun) { - String errorMessage = "The server is stopped."; - LOG.warn(errorMessage); - throw new IOException(errorMessage); - } - if (reconfigThread != null) { - String errorMessage = "Another reconfiguration task is running."; - LOG.warn(errorMessage); - throw new IOException(errorMessage); - } - reconfigThread = new ReconfigurationThread(this); - reconfigThread.setDaemon(true); - reconfigThread.setName("Reconfiguration Task"); - reconfigThread.start(); - startTime = System.nanoTime(); - } + public void startReconfiguration() throws IOException { + context.start(); } - public ReconfigurationTaskStatus getReconfigurationTaskStatus() { - synchronized (reconfigLock) { - if (reconfigThread != null) { - return new ReconfigurationTaskStatus(startTime, 0, null); - } - return new ReconfigurationTaskStatus(startTime, endTime, status); - } + public ReconfigurationStatus getReconfigurationStatus() { + return context.getStatus(); } - public void shutdownReconfigurationTask() { - Thread tempThread; - synchronized (reconfigLock) { - shouldRun = false; - if (reconfigThread == null) { - return; - } - tempThread = reconfigThread; - reconfigThread = null; - } - - try { - tempThread.join(); - } catch (InterruptedException e) { - } + public void shutdown() throws InterruptedException { + context.stop().join(); } /** - * {@inheritDoc} - * - * This method makes the change to this objects {@link RaftProperties} - * and calls reconfigurePropertyImpl to update internal data structures. - * This method cannot be overridden, subclasses should instead override - * reconfigurePropertyImpl. + * Run a batch reconfiguration to change the current properties + * to the properties returned by {@link #getNewProperties()}. */ - @Override - public final void reconfigureProperty(String property, String newVal) - throws ReconfigurationException { - if (isPropertyReconfigurable(property)) { - LOG.info("changing property " + property + " to " + newVal); - synchronized(getConf()) { - getConf().get(property); - String effectiveValue = reconfigurePropertyImpl(property, newVal); - if (newVal != null) { - getConf().set(property, effectiveValue); - } else { - getConf().unset(property); - } + private void batchReconfiguration() { + LOG.info("{}: Starting batch reconfiguration {}", name, Thread.currentThread()); + final Collection changes = getChangedProperties(getNewProperties(), properties); + final Map results = new HashMap<>(); + for (PropertyChange change : changes) { + LOG.info("Change property: " + change); + try { + singleReconfiguration(change.getProperty(), change.getNewValue()); + results.put(change, null); + } catch (Throwable t) { + results.put(change, t); } - } else { - throw new ReconfigurationException(property, newVal, - getConf().get(property)); } + context.end(results); } - /** - * {@inheritDoc} - * - * Subclasses must override this. - */ - @Override - public abstract Collection getReconfigurableProperties(); - - - /** - * {@inheritDoc} - * - * Subclasses may wish to override this with a more efficient implementation. - */ - @Override - public boolean isPropertyReconfigurable(String property) { - return getReconfigurableProperties().contains(property); + /** Run a single reconfiguration to change the given property to the given value. */ + private void singleReconfiguration(String property, String newValue) throws ReconfigurationException { + if (!isPropertyReconfigurable(property)) { + throw new ReconfigurationException("Property is not reconfigurable.", + property, newValue, properties.get(property)); + } + final String effective = reconfigureProperty(property, newValue); + LOG.info("{}: changed property {} to {} (effective {})", name, property, newValue, effective); + if (newValue != null) { + properties.set(property, effective); + } else { + properties.unset(property); + } } - - /** - * Change a configuration property. - * - * Subclasses must override this. This method applies the change to - * all internal data structures derived from the configuration property - * that is being changed. If this object owns other Reconfigurable objects - * reconfigureProperty should be called recursively to make sure that - * the configuration of these objects are updated. - * - * @param property Name of the property that is being reconfigured. - * @param newVal Proposed new value of the property. - * @return Effective new value of the property. This may be different from - * newVal. - * - * @throws ReconfigurationException if there was an error applying newVal. - */ - protected abstract String reconfigurePropertyImpl(String property, String newVal) - throws ReconfigurationException; - -} +} \ No newline at end of file diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java index 192f2aebc0..15c8c82254 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationException.java @@ -18,87 +18,46 @@ package org.apache.ratis.conf; -public class ReconfigurationException extends Exception{ +import static org.apache.ratis.conf.ReconfigurationStatus.propertyString; +public class ReconfigurationException extends Exception { private static final long serialVersionUID = 1L; - private String property; - private String newVal; - private String oldVal; - - /** - * Construct the exception message. - */ - private static String constructMessage(String property, String newVal, String oldVal) { - String message = "Could not change property " + property; - if (oldVal != null) { - message += " from \'" + oldVal; - } - if (newVal != null) { - message += "\' to \'" + newVal + "\'"; - } - return message; - } - - - /** - * Create a new instance of {@link ReconfigurationException}. - */ - public ReconfigurationException() { - super("Could not change configuration."); - this.property = null; - this.newVal = null; - this.oldVal = null; - } + private final String property; + private final String newValue; + private final String oldValue; /** * Create a new instance of {@link ReconfigurationException}. - * @param property property name. - * @param newVal new value. - * @param oldVal old value. - * @param cause original exception. + * @param property the property name. + * @param newValue the new value. + * @param oldValue the old value. + * @param cause the cause of this exception. */ - public ReconfigurationException(String property, String newVal, String oldVal, Throwable cause) { - super(constructMessage(property, newVal, oldVal), cause); + public ReconfigurationException(String reason, String property, String newValue, String oldValue, Throwable cause) { + super("Failed to change property " + propertyString(property, newValue, oldValue) + ": " + reason, cause); this.property = property; - this.newVal = newVal; - this.oldVal = oldVal; + this.newValue = newValue; + this.oldValue = oldValue; } - /** - * Create a new instance of {@link ReconfigurationException}. - * @param property property name. - * @param newVal new value. - * @param oldVal old value. - */ - public ReconfigurationException(String property, String newVal, String oldVal) { - super(constructMessage(property, newVal, oldVal)); - this.property = property; - this.newVal = newVal; - this.oldVal = oldVal; + /** The same as new ReconfigurationException(reason, property, newValue, oldValue, null). */ + public ReconfigurationException(String reason, String property, String newValue, String oldValue) { + this(reason, property, newValue, oldValue, null); } - /** - * Get property that cannot be changed. - * @return property info. - */ + /** @return the property name related to this exception. */ public String getProperty() { return property; } - /** - * Get value to which property was supposed to be changed. - * @return new value. - */ + /** @return the value that the property was supposed to be changed. */ public String getNewValue() { - return newVal; + return newValue; } - /** - * Get old value of property that cannot be changed. - * @return old value. - */ + /** @return the old value of the property. */ public String getOldValue() { - return oldVal; + return oldValue; } } diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationStatus.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationStatus.java new file mode 100644 index 0000000000..c584fe068a --- /dev/null +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationStatus.java @@ -0,0 +1,151 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ratis.conf; + +import java.util.Map; +import java.util.Objects; + +import org.apache.ratis.util.Daemon; +import org.apache.ratis.util.Timestamp; + +/** The status of a reconfiguration task. */ +public class ReconfigurationStatus { + private static String quote(String value) { + return value == null? "": "\"" + value + "\""; + } + + static String propertyString(String property, String newValue, String oldValue) { + Objects.requireNonNull(property, "property == null"); + return property + " from " + quote(oldValue) + " to " + quote(newValue); + } + + /** The change of a configuration property. */ + public static class PropertyChange { + private final String property; + private final String newValue; + private final String oldValue; + + public PropertyChange(String property, String newValue, String oldValue) { + this.property = property; + this.newValue = newValue; + this.oldValue = oldValue; + } + + /** @return the name of the property being changed. */ + public String getProperty() { + return property; + } + + /** @return the new value to be changed to. */ + public String getNewValue() { + return newValue; + } + + /** @return the old value of the property. */ + public String getOldValue() { + return oldValue; + } + + @Override + public int hashCode() { + return property.hashCode(); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } else if (!(obj instanceof PropertyChange)) { + return false; + } + final PropertyChange that = (PropertyChange)obj; + return Objects.equals(this.property, that.property) + && Objects.equals(this.oldValue, that.oldValue) + && Objects.equals(this.newValue, that.newValue); + } + + @Override + public String toString() { + return propertyString(getProperty(), getNewValue(), getOldValue()); + } + } + + /** The timestamp when the reconfiguration starts. */ + private final Timestamp startTime; + /** The timestamp when the reconfiguration completes. */ + private final Timestamp endTime; + /** + * A property-change map. + * For a particular change, if the error is null, + * it indicates that the change has been applied successfully. + * Otherwise, it is the error occurred when applying the change. + */ + private final Map changes; + /** The daemon to run the reconfiguration. */ + private final Daemon daemon; + + ReconfigurationStatus(Timestamp startTime, Timestamp endTime, Map changes, Daemon daemon) { + this.startTime = startTime; + this.endTime = endTime; + this.changes = changes; + this.daemon = daemon; + } + + /** @return true iff a reconfiguration task has started (it may either be running or already has finished). */ + public boolean started() { + return getStartTime() != null; + } + + /** @return true if the latest reconfiguration task has ended and there are no new active tasks started. */ + public boolean ended() { + return getEndTime() != null; + } + + /** + * @return the start time of the reconfiguration task if the reconfiguration task has been started; + * otherwise, return null. + */ + public Timestamp getStartTime() { + return startTime; + } + + /** + * @return the end time of the reconfiguration task if the reconfiguration task has been ended; + * otherwise, return null. + */ + public Timestamp getEndTime() { + return endTime; + } + + /** + * @return the changes of the reconfiguration task if the reconfiguration task has been ended; + * otherwise, return null. + */ + public Map getChanges() { + return changes; + } + + /** + * @return the daemon running the reconfiguration task if the task has been started; + * otherwise, return null. + */ + Daemon getDaemon() { + return daemon; + } +} diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java deleted file mode 100644 index 7efc2d9dbf..0000000000 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationTaskStatus.java +++ /dev/null @@ -1,67 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.ratis.conf; - -import java.util.Map; -import java.util.Optional; -import org.apache.ratis.conf.ReconfigurationUtil.PropertyChange; - -public class ReconfigurationTaskStatus { - private long startTime; - private long endTime; - private final Map> status; - - public ReconfigurationTaskStatus(long startTime, long endTime, - Map> status) { - this.startTime = startTime; - this.endTime = endTime; - this.status = status; - } - - /** - * Return true if - * - A reconfiguration task has finished or - * - an active reconfiguration task is running. - * @return true if startTime > 0; false if not. - */ - public boolean hasTask() { - return startTime > 0; - } - - /** - * Return true if the latest reconfiguration task has finished and there is - * no another active task running. - * @return true if endTime > 0; false if not. - */ - public boolean stopped() { - return endTime > 0; - } - - public long getStartTime() { - return startTime; - } - - public long getEndTime() { - return endTime; - } - - public final Map> getStatus() { - return status; - } -} diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java deleted file mode 100644 index a2f0ee92e3..0000000000 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationUtil.java +++ /dev/null @@ -1,82 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one - * or more contributor license agreements. See the NOTICE file - * distributed with this work for additional information - * regarding copyright ownership. The ASF licenses this file - * to you under the Apache License, Version 2.0 (the - * "License"); you may not use this file except in compliance - * with the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.apache.ratis.conf; - -import java.util.Collection; -import java.util.HashMap; -import java.util.Map; - -public class ReconfigurationUtil { - public static class PropertyChange { - private final String prop; - private final String oldVal; - private final String newVal; - - public PropertyChange(String prop, String newVal, String oldVal) { - this.prop = prop; - this.newVal = newVal; - this.oldVal = oldVal; - } - - public String getProp() { - return prop; - } - - public String getOldVal() { - return oldVal; - } - - public String getNewVal() { - return newVal; - } - } - - public static Collection - getChangedProperties(RaftProperties newConf, RaftProperties oldConf) { - Map changes = new HashMap(); - - // iterate over old configuration - for (Map.Entry oldEntry: oldConf.getProperties().entrySet()) { - String prop = oldEntry.getKey(); - String oldVal = oldEntry.getValue(); - String newVal = newConf.getRaw(prop); - - if (newVal == null || !newVal.equals(oldVal)) { - changes.put(prop, new PropertyChange(prop, newVal, oldVal)); - } - } - - // now iterate over new configuration - // (to look for properties not present in old conf) - for (Map.Entry newEntry: newConf.getProperties().entrySet()) { - String prop = newEntry.getKey(); - String newVal = newEntry.getValue(); - if (oldConf.get(prop) == null) { - changes.put(prop, new PropertyChange(prop, newVal, null)); - } - } - - return changes.values(); - } - - public Collection parseChangedProperties( - RaftProperties newConf, RaftProperties oldConf) { - return getChangedProperties(newConf, oldConf); - } -} diff --git a/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java b/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java index 587c785f52..4535406a77 100644 --- a/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java +++ b/ratis-server/src/test/java/org/apache/ratis/TestReConfigProperty.java @@ -18,26 +18,14 @@ package org.apache.ratis; -import static org.junit.Assert.fail; -import static org.junit.matchers.JUnitMatchers.containsString; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.doReturn; -import static org.mockito.Mockito.doThrow; -import static org.mockito.Mockito.spy; - import org.apache.ratis.client.impl.OrderedAsync; import org.apache.ratis.conf.RaftProperties; import org.apache.ratis.conf.ReconfigurationBase; import org.apache.ratis.conf.ReconfigurationException; -import org.apache.ratis.conf.ReconfigurationTaskStatus; -import org.apache.ratis.conf.ReconfigurationUtil; -import org.apache.ratis.conf.ReconfigurationUtil.PropertyChange; +import org.apache.ratis.conf.ReconfigurationStatus.PropertyChange; import org.apache.ratis.server.impl.MiniRaftCluster; import org.apache.ratis.statemachine.StateMachine; import org.apache.ratis.statemachine.impl.SimpleStateMachine4Testing; -import org.apache.ratis.thirdparty.com.google.common.collect.Lists; import org.apache.ratis.util.Slf4jUtils; import org.junit.Assert; import org.junit.Before; @@ -47,10 +35,6 @@ import java.io.IOException; import java.util.Arrays; import java.util.Collection; -import java.util.List; -import java.util.Map; -import java.util.Optional; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeoutException; public abstract class TestReConfigProperty extends BaseTest @@ -73,7 +57,7 @@ public abstract class TestReConfigProperty exte private static final String VAL1 = "val1"; private static final String VAL2 = "val2"; - + private static final String DEFAULT = "default"; @Before public void setup () { @@ -94,8 +78,8 @@ public void setup () { @Test public void testGetChangedProperty() { - Collection changes - = ReconfigurationUtil.getChangedProperties(conf2, conf1); + Collection changes + = ReconfigurationBase.getChangedProperties(conf2, conf1); Assert.assertTrue("expected 3 changed properties but got " + changes.size(), changes.size() == 3); @@ -104,15 +88,15 @@ public void testGetChangedProperty() { boolean unsetFound = false; boolean setFound = false; - for (ReconfigurationUtil.PropertyChange c: changes) { - if (c.getProp().equals(PROP2) && c.getOldVal() != null && c.getOldVal().equals(VAL1) && - c.getNewVal() != null && c.getNewVal().equals(VAL2)) { + for (PropertyChange c: changes) { + if (c.getProperty().equals(PROP2) && c.getOldValue() != null && c.getOldValue().equals(VAL1) && + c.getNewValue() != null && c.getNewValue().equals(VAL2)) { changeFound = true; - } else if (c.getProp().equals(PROP3) && c.getOldVal() != null && c.getOldVal().equals(VAL1) && - c.getNewVal() == null) { + } else if (c.getProperty().equals(PROP3) && c.getOldValue() != null && c.getOldValue().equals(VAL1) && + c.getNewValue() == null) { unsetFound = true; - } else if (c.getProp().equals(PROP4) && c.getOldVal() == null && - c.getNewVal() != null && c.getNewVal().equals(VAL1)) { + } else if (c.getProperty().equals(PROP4) && c.getOldValue() == null && + c.getNewValue() != null && c.getNewValue().equals(VAL1)) { setFound = true; } } @@ -126,26 +110,28 @@ public void testGetChangedProperty() { public static class ReconfigurableDummy extends ReconfigurationBase implements Runnable { public volatile boolean running = true; + private RaftProperties newProp; public ReconfigurableDummy(RaftProperties prop) { - super(prop); + super("reConfigDummy", prop); } @Override - protected RaftProperties getNewConf() { - return new RaftProperties(); + protected RaftProperties getNewProperties() { + return newProp; } @Override - public Collection getReconfigurableProperties() { - return Arrays.asList(PROP1, PROP2, PROP4); + public synchronized String reconfigureProperty(String property, String newValue) + throws ReconfigurationException { + newProp = new RaftProperties(); + newProp.set(property, newValue != null ? newValue : DEFAULT); + return newValue; } @Override - public synchronized String reconfigurePropertyImpl( - String property, String newVal) throws ReconfigurationException { - // do nothing - return newVal; + public Collection getReconfigurableProperties() { + return Arrays.asList(PROP1, PROP2, PROP4); } /** @@ -153,7 +139,7 @@ public synchronized String reconfigurePropertyImpl( */ @Override public void run() { - while (running && getConf().get(PROP1).equals(VAL1)) { + while (running && getProperties().get(PROP1).equals(VAL1)) { try { Thread.sleep(1); } catch (InterruptedException ignore) { @@ -171,16 +157,11 @@ public void run() { public void testReconfigure() { ReconfigurableDummy dummy = new ReconfigurableDummy(conf1); - Assert.assertTrue(PROP1 + " set to wrong value ", - dummy.getConf().get(PROP1).equals(VAL1)); - Assert.assertTrue(PROP2 + " set to wrong value ", - dummy.getConf().get(PROP2).equals(VAL1)); - Assert.assertTrue(PROP3 + " set to wrong value ", - dummy.getConf().get(PROP3).equals(VAL1)); - Assert.assertTrue(PROP4 + " set to wrong value ", - dummy.getConf().get(PROP4) == null); - Assert.assertTrue(PROP5 + " set to wrong value ", - dummy.getConf().get(PROP5) == null); + Assert.assertEquals(PROP1 + " set to wrong value ", VAL1, dummy.getProperties().get(PROP1)); + Assert.assertEquals(PROP2 + " set to wrong value ", VAL1, dummy.getProperties().get(PROP2)); + Assert.assertEquals(PROP3 + " set to wrong value ", VAL1, dummy.getProperties().get(PROP3)); + Assert.assertNull(PROP4 + " set to wrong value ", dummy.getProperties().get(PROP4)); + Assert.assertNull(PROP5 + " set to wrong value ", dummy.getProperties().get(PROP5)); Assert.assertTrue(PROP1 + " should be reconfigurable ", dummy.isPropertyReconfigurable(PROP1)); @@ -198,9 +179,10 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP1, VAL1); - Assert.assertTrue(PROP1 + " set to wrong value ", - dummy.getConf().get(PROP1).equals(VAL1)); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertEquals(PROP1 + " set to wrong value ", VAL1, dummy.getProperties().get(PROP1)); + } catch (ReconfigurationException | IOException | TimeoutException | InterruptedException e) { exceptionCaught = true; } Assert.assertFalse("received unexpected exception", @@ -212,9 +194,11 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP1, null); - Assert.assertTrue(PROP1 + "set to wrong value ", - dummy.getConf().get(PROP1) == null); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertEquals(PROP1 + "set to wrong value ", DEFAULT, + dummy.getProperties().get(PROP1)); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertFalse("received unexpected exception", @@ -226,9 +210,10 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP1, VAL2); - Assert.assertTrue(PROP1 + "set to wrong value ", - dummy.getConf().get(PROP1).equals(VAL2)); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertEquals(PROP1 + "set to wrong value ", VAL2, dummy.getProperties().get(PROP1)); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertFalse("received unexpected exception", @@ -240,9 +225,10 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP4, null); - Assert.assertTrue(PROP4 + "set to wrong value ", - dummy.getConf().get(PROP4) == null); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertSame(PROP4 + "set to wrong value ", DEFAULT, dummy.getProperties().get(PROP4)); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertFalse("received unexpected exception", @@ -254,9 +240,10 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP4, VAL1); - Assert.assertTrue(PROP4 + "set to wrong value ", - dummy.getConf().get(PROP4).equals(VAL1)); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertEquals(PROP4 + "set to wrong value ", VAL1, dummy.getProperties().get(PROP4)); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertFalse("received unexpected exception", @@ -268,11 +255,15 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP5, null); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertTrue("did not receive expected exception", - exceptionCaught); + dummy.getReconfigurationStatus().getChanges() + .get(new PropertyChange(PROP5, DEFAULT, null)) + .getMessage().contains("Property is not reconfigurable.") && !exceptionCaught); } // try to set unset property to value (not reconfigurable) @@ -280,11 +271,15 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP5, VAL1); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertTrue("did not receive expected exception", - exceptionCaught); + dummy.getReconfigurationStatus().getChanges() + .get(new PropertyChange(PROP5, VAL1, null)) + .getMessage().contains("Property is not reconfigurable.") && !exceptionCaught); } // try to change property to value (not reconfigurable) @@ -292,11 +287,15 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP3, VAL2); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertTrue("did not receive expected exception", - exceptionCaught); + dummy.getReconfigurationStatus().getChanges() + .get(new PropertyChange(PROP3, VAL2, VAL1)) + .getMessage().contains("Property is not reconfigurable.") && !exceptionCaught); } // try to change property to null (not reconfigurable) @@ -304,11 +303,15 @@ public void testReconfigure() { boolean exceptionCaught = false; try { dummy.reconfigureProperty(PROP3, null); - } catch (ReconfigurationException e) { + dummy.startReconfiguration(); + RaftTestUtil.waitFor(() -> dummy.getReconfigurationStatus().ended(), 100, 60000); + } catch (ReconfigurationException | IOException | InterruptedException | TimeoutException e) { exceptionCaught = true; } Assert.assertTrue("did not receive expected exception", - exceptionCaught); + dummy.getReconfigurationStatus().getChanges() + .get(new PropertyChange(PROP3, DEFAULT, VAL1)) + .getMessage().contains("Property is not reconfigurable.") && !exceptionCaught); } } @@ -316,9 +319,9 @@ public void testReconfigure() { * Test whether configuration changes are visible in another thread. */ @Test - public void testThread() throws ReconfigurationException { + public void testThread() throws ReconfigurationException, IOException { ReconfigurableDummy dummy = new ReconfigurableDummy(conf1); - Assert.assertTrue(dummy.getConf().get(PROP1).equals(VAL1)); + Assert.assertEquals(VAL1, dummy.getProperties().get(PROP1)); Thread dummyThread = new Thread(dummy); dummyThread.start(); try { @@ -327,6 +330,7 @@ public void testThread() throws ReconfigurationException { // do nothing } dummy.reconfigureProperty(PROP1, VAL2); + dummy.startReconfiguration(); long endWait = System.currentTimeMillis() + 2000; while (dummyThread.isAlive() && System.currentTimeMillis() < endWait) { @@ -346,157 +350,18 @@ public void testThread() throws ReconfigurationException { // do nothing } Assert.assertTrue(PROP1 + " is set to wrong value", - dummy.getConf().get(PROP1).equals(VAL2)); + dummy.getProperties().get(PROP1).equals(VAL2)); } - private static class AsyncReconfigurableDummy extends ReconfigurationBase { - - AsyncReconfigurableDummy(RaftProperties prop) { - super(prop); - } - - @Override - protected RaftProperties getNewConf() { - return new RaftProperties(); - } - - final CountDownLatch latch = new CountDownLatch(1); - - @Override - public Collection getReconfigurableProperties() { - return Arrays.asList(PROP1, PROP2, PROP4); - } - - @Override - public synchronized String reconfigurePropertyImpl(String property, - String newVal) throws ReconfigurationException { - try { - latch.await(); - } catch (InterruptedException e) { - // Ignore - } - return newVal; - } - } - - private static void waitAsyncReconfigureTaskFinish(ReconfigurationBase rb) - throws InterruptedException { - ReconfigurationTaskStatus status = null; - int count = 20; - while (count > 0) { - status = rb.getReconfigurationTaskStatus(); - if (status.stopped()) { - break; - } - count--; - Thread.sleep(500); - } - assert(status.stopped()); - } - - @Test - public void testAsyncReconfigure() - throws ReconfigurationException, IOException, InterruptedException { - AsyncReconfigurableDummy dummy = spy(new AsyncReconfigurableDummy(conf1)); - - List changes = Lists.newArrayList(); - changes.add(new PropertyChange("name1", "new1", "old1")); - changes.add(new PropertyChange("name2", "new2", "old2")); - changes.add(new PropertyChange("name3", "new3", "old3")); - doReturn(changes).when(dummy).getChangedProperties( - any(RaftProperties.class), any(RaftProperties.class)); - - doReturn(true).when(dummy).isPropertyReconfigurable(eq("name1")); - doReturn(false).when(dummy).isPropertyReconfigurable(eq("name2")); - doReturn(true).when(dummy).isPropertyReconfigurable(eq("name3")); - - doReturn("dummy").when(dummy) - .reconfigurePropertyImpl(eq("name1"), anyString()); - doReturn("dummy").when(dummy) - .reconfigurePropertyImpl(eq("name2"), anyString()); - doThrow(new ReconfigurationException("NAME3", "NEW3", "OLD3", - new IOException("io exception"))) - .when(dummy).reconfigurePropertyImpl(eq("name3"), anyString()); - - dummy.startReconfigurationTask(); - - waitAsyncReconfigureTaskFinish(dummy); - ReconfigurationTaskStatus status = dummy.getReconfigurationTaskStatus(); - Assert.assertEquals(2, status.getStatus().size()); - for (Map.Entry> result : - status.getStatus().entrySet()) { - PropertyChange change = result.getKey(); - if (change.getProp().equals("name1")) { - Assert.assertFalse(result.getValue().isPresent()); - } else if (change.getProp().equals("name2")) { - Assert.assertThat(result.getValue().get(), - containsString("Property name2 is not reconfigurable")); - } else if (change.getProp().equals("name3")) { - Assert.assertThat(result.getValue().get(), containsString("io exception")); - } else { - fail("Unknown property: " + change.getProp()); - } - } - } - - @Test(timeout=30000) - public void testStartReconfigurationFailureDueToExistingRunningTask() - throws InterruptedException, IOException { - AsyncReconfigurableDummy dummy = spy(new AsyncReconfigurableDummy(conf1)); - List changes = Lists.newArrayList( - new PropertyChange(PROP1, "new1", "old1") - ); - doReturn(changes).when(dummy).getChangedProperties( - any(RaftProperties.class), any(RaftProperties.class)); - - ReconfigurationTaskStatus status = dummy.getReconfigurationTaskStatus(); - Assert.assertFalse(status.hasTask()); - - dummy.startReconfigurationTask(); - status = dummy.getReconfigurationTaskStatus(); - Assert.assertTrue(status.hasTask()); - Assert.assertFalse(status.stopped()); - - // An active reconfiguration task is running. - try { - dummy.startReconfigurationTask(); - fail("Expect to throw IOException."); - } catch (IOException e) { - Assert.assertTrue(e.getMessage().contains("Another reconfiguration task is running")); - } - status = dummy.getReconfigurationTaskStatus(); - Assert.assertTrue(status.hasTask()); - Assert.assertFalse(status.stopped()); - - dummy.latch.countDown(); - waitAsyncReconfigureTaskFinish(dummy); - status = dummy.getReconfigurationTaskStatus(); - Assert.assertTrue(status.hasTask()); - Assert.assertTrue(status.stopped()); - - // The first task has finished. - dummy.startReconfigurationTask(); - waitAsyncReconfigureTaskFinish(dummy); - ReconfigurationTaskStatus status2 = dummy.getReconfigurationTaskStatus(); - Assert.assertTrue(status2.getStartTime() >= status.getEndTime()); - - dummy.shutdownReconfigurationTask(); - try { - dummy.startReconfigurationTask(); - fail("Expect to throw IOException"); - } catch (IOException e) { - Assert.assertTrue(e.getMessage().contains("The server is stopped")); - } - } - /** * Ensure that {@link ReconfigurationBase#reconfigureProperty} updates the * parent's cached configuration on success. * @throws IOException */ @Test (timeout=300000) - public void testConfIsUpdatedOnSuccess() throws ReconfigurationException { + public void testConfIsUpdatedOnSuccess() + throws ReconfigurationException, IOException, InterruptedException, TimeoutException { final String property = "FOO"; final String value1 = "value1"; final String value2 = "value2"; @@ -510,11 +375,13 @@ public void testConfIsUpdatedOnSuccess() throws ReconfigurationException { conf, newConf, Arrays.asList(property)); reconfigurable.reconfigureProperty(property, value2); - Assert.assertEquals(value2, reconfigurable.getConf().get(property)); + reconfigurable.startReconfiguration(); + RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertEquals(value2, reconfigurable.getProperties().get(property)); } /** - * Ensure that {@link ReconfigurationBase#startReconfigurationTask} updates + * Ensure that {@link ReconfigurationBase#startReconfiguration} updates * its parent's cached configuration on success. * @throws IOException */ @@ -534,10 +401,10 @@ public void testConfIsUpdatedOnSuccessAsync() conf, newConf, Arrays.asList(property)); // Kick off a reconfiguration task and wait until it completes. - reconfigurable.startReconfigurationTask(); + reconfigurable.startReconfiguration(); - RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationTaskStatus().stopped(), 100, 60000); - Assert.assertEquals(value2, reconfigurable.getConf().get(property)); + RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertEquals(value2, reconfigurable.getProperties().get(property)); } /** @@ -546,7 +413,8 @@ public void testConfIsUpdatedOnSuccessAsync() * @throws IOException */ @Test (timeout=300000) - public void testConfIsUnset() throws ReconfigurationException { + public void testConfIsUnset() + throws InterruptedException, TimeoutException, IOException { final String property = "FOO"; final String value1 = "value1"; @@ -557,12 +425,13 @@ public void testConfIsUnset() throws ReconfigurationException { final ReconfigurationBase reconfigurable = makeReconfigurable( conf, newConf, Arrays.asList(property)); - reconfigurable.reconfigureProperty(property, null); - Assert.assertNull(reconfigurable.getConf().get(property)); + reconfigurable.startReconfiguration(); + RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertNull(reconfigurable.getProperties().get(property)); } /** - * Ensure that {@link ReconfigurationBase#startReconfigurationTask} unsets the + * Ensure that {@link ReconfigurationBase#startReconfiguration} unsets the * property in its parent's configuration when the new value is null. * @throws IOException */ @@ -580,31 +449,29 @@ public void testConfIsUnsetAsync() throws ReconfigurationException, conf, newConf, Arrays.asList(property)); // Kick off a reconfiguration task and wait until it completes. - reconfigurable.startReconfigurationTask(); - RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationTaskStatus().stopped(), 100, 60000); - Assert.assertNull(reconfigurable.getConf().get(property)); + reconfigurable.startReconfiguration(); + RaftTestUtil.waitFor(() -> reconfigurable.getReconfigurationStatus().ended(), 100, 60000); + Assert.assertNull(reconfigurable.getProperties().get(property)); } private ReconfigurationBase makeReconfigurable( - final RaftProperties oldConf, final RaftProperties newConf, + final RaftProperties oldProperties, final RaftProperties newProperties, final Collection reconfigurableProperties) { - final RaftProperties prop; - return new ReconfigurationBase(oldConf) { + return new ReconfigurationBase("tempReConfigDummy", oldProperties) { @Override - protected RaftProperties getNewConf() { - return newConf; + protected RaftProperties getNewProperties() { + return newProperties; } @Override - public Collection getReconfigurableProperties() { - return reconfigurableProperties; + public String reconfigureProperty(String property, String newValue) { + return newValue; } @Override - protected String reconfigurePropertyImpl( - String property, String newVal) throws ReconfigurationException { - return newVal; + public Collection getReconfigurableProperties() { + return reconfigurableProperties; } }; } From 862ca0f8aa6e266c38d9996e29f54d5cd77bf414 Mon Sep 17 00:00:00 2001 From: dragonyliu Date: Fri, 30 Dec 2022 19:15:04 +0800 Subject: [PATCH 4/4] fix checkstyle --- .../main/java/org/apache/ratis/conf/ReconfigurationBase.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java index 7b0f2c8a20..ea6ba225e4 100644 --- a/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java +++ b/ratis-common/src/main/java/org/apache/ratis/conf/ReconfigurationBase.java @@ -40,7 +40,8 @@ public abstract class ReconfigurationBase implements Reconfigurable { private static final Logger LOG = LoggerFactory.getLogger(ReconfigurationBase.class); - public static Collection getChangedProperties(RaftProperties newProperties, RaftProperties oldProperties) { + public static Collection getChangedProperties( + RaftProperties newProperties, RaftProperties oldProperties) { final Map changes = new HashMap<>(); // iterate over old properties