diff --git a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java index 2fa5e90e0b5..6c84b9adf04 100644 --- a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java +++ b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java @@ -27,10 +27,16 @@ import io.netty.buffer.ByteBufAllocator; import java.io.IOException; import java.lang.Thread.UncaughtExceptionHandler; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; +import java.lang.reflect.Modifier; import java.net.UnknownHostException; import java.util.Arrays; -import java.util.Iterator; +import java.util.HashSet; +import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.Set; import java.util.TreeMap; import lombok.CustomLog; import org.apache.bookkeeper.bookie.Bookie; @@ -39,6 +45,7 @@ import org.apache.bookkeeper.bookie.BookieImpl; import org.apache.bookkeeper.bookie.ExitCode; import org.apache.bookkeeper.bookie.UncleanShutdownDetection; +import org.apache.bookkeeper.conf.AbstractConfiguration; import org.apache.bookkeeper.conf.ServerConfiguration; import org.apache.bookkeeper.net.BookieId; import org.apache.bookkeeper.net.BookieSocketAddress; @@ -220,28 +227,93 @@ private void validateUser(ServerConfiguration conf) throws BookieException { } /** - * Returns the configuration entries that differ from the {@link ServerConfiguration} - * defaults — i.e. values explicitly set by the user (via config file or programmatically) - * whose value is not the same as the built-in default. + * Returns the entries of {@code conf} whose value differs from the built-in default, keyed by + * configuration property name. + * + *

The defaults are not stored in the configuration but encoded in the {@link ServerConfiguration} + * getters, so every getter is invoked on both {@code conf} and an empty configuration, recording the + * keys it reads. When the two results differ, the recorded keys that are explicitly set in {@code conf} + * are reported. Keys that no getter reads (settings of other components, or unrelated entries loaded + * from a shared properties file) are not reported. */ - private static Map overriddenConfig(ServerConfiguration conf) { - ServerConfiguration defaults = new ServerConfiguration(); + @VisibleForTesting + static Map overriddenConfig(ServerConfiguration conf) { + KeyRecordingConfiguration actual = new KeyRecordingConfiguration(conf); + KeyRecordingConfiguration defaults = new KeyRecordingConfiguration(new ServerConfiguration()); Map overrides = new TreeMap<>(); - Iterator keys = conf.getInMemoryConfiguration().getKeys(); - while (keys.hasNext()) { - String key = keys.next(); - Object value = conf.getProperty(key); - if (value == null) { + for (Method getter : ServerConfiguration.class.getMethods()) { + if (!isGetter(getter)) { continue; } - Object defaultValue = defaults.getProperty(key); - if (defaultValue == null || !value.toString().equals(defaultValue.toString())) { - overrides.put(key, value); + actual.keys.clear(); + defaults.keys.clear(); + if (Objects.deepEquals(invoke(getter, actual), invoke(getter, defaults))) { + continue; + } + Set keys = new HashSet<>(actual.keys); + keys.addAll(defaults.keys); + for (String key : keys) { + if (conf.containsKey(key)) { + overrides.put(key, conf.getProperty(key)); + } } } return overrides; } + private static boolean isGetter(Method method) { + return method.getParameterCount() == 0 + && !Modifier.isStatic(method.getModifiers()) + && AbstractConfiguration.class.isAssignableFrom(method.getDeclaringClass()) + && (method.getName().startsWith("get") || method.getName().startsWith("is")); + } + + private static Object invoke(Method getter, ServerConfiguration conf) { + try { + return getter.invoke(conf); + } catch (InvocationTargetException e) { + // a getter rejecting the configured value still means the value differs from the default + return e.getCause().getClass(); + } catch (IllegalAccessException e) { + throw new IllegalStateException(e); + } + } + + /** + * A {@link ServerConfiguration} that reads its properties from another configuration and records the + * keys that have been looked up. + * + *

Besides the two internal hooks, {@code getList} must be overridden because + * {@code CompositeConfiguration} implements it (and {@code getStringArray}) by reading its child + * configurations directly. + */ + private static class KeyRecordingConfiguration extends ServerConfiguration { + private final ServerConfiguration source; + private final Set keys = new HashSet<>(); + + KeyRecordingConfiguration(ServerConfiguration source) { + this.source = source; + } + + @Override + protected Object getPropertyInternal(String key) { + keys.add(key); + return source.getProperty(key); + } + + @Override + protected boolean containsKeyInternal(String key) { + keys.add(key); + return source.containsKey(key); + } + + @Override + public List getList(String key, List defaultValue) { + keys.add(key); + return source.getList(key, defaultValue); + } + } + public boolean isRunning() { return bookie.isRunning() && nettyServer.isRunning() && running; diff --git a/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieServerOverriddenConfigTest.java b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieServerOverriddenConfigTest.java new file mode 100644 index 00000000000..3ffc637631c --- /dev/null +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieServerOverriddenConfigTest.java @@ -0,0 +1,98 @@ +/* + * + * 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.bookkeeper.proto; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +import java.util.Map; +import java.util.Set; +import org.apache.bookkeeper.conf.ServerConfiguration; +import org.junit.Test; + +/** + * Unit test for {@link BookieServer#overriddenConfig(ServerConfiguration)}. + */ +public class BookieServerOverriddenConfigTest { + + @Test + public void testEmptyConfiguration() { + assertTrue(BookieServer.overriddenConfig(new ServerConfiguration()).isEmpty()); + } + + @Test + public void testValuesEqualToDefaultsAreNotReported() { + ServerConfiguration conf = new ServerConfiguration(); + conf.setBookiePort(3181); + conf.setJournalSyncData(true); + conf.setGcWaitTime(600000); + conf.setJournalDirName("/tmp/bk-txn"); + + assertTrue(BookieServer.overriddenConfig(conf).isEmpty()); + } + + @Test + public void testOverriddenValuesAreReported() { + ServerConfiguration conf = new ServerConfiguration(); + conf.setBookiePort(3181); + conf.setJournalSyncData(false); + conf.setGcWaitTime(300000); + conf.setLedgerDirNames(new String[] { "/data/ledgers1", "/data/ledgers2" }); + + Map overrides = BookieServer.overriddenConfig(conf); + + assertEquals(Set.of("journalSyncData", "gcWaitTime", "ledgerDirectories"), overrides.keySet()); + assertEquals("false", String.valueOf(overrides.get("journalSyncData"))); + assertEquals("300000", String.valueOf(overrides.get("gcWaitTime"))); + assertEquals("[/data/ledgers1, /data/ledgers2]", String.valueOf(overrides.get("ledgerDirectories"))); + } + + @Test + public void testOnlyKeysActuallySetAreReported() { + ServerConfiguration conf = new ServerConfiguration(); + // getJournalDirNames() looks up journalDirectories first and falls back to journalDirectory + conf.setProperty("journalDirectory", "/data/journal"); + + Map overrides = BookieServer.overriddenConfig(conf); + + assertEquals(Set.of("journalDirectory"), overrides.keySet()); + assertEquals("/data/journal", overrides.get("journalDirectory")); + } + + @Test + public void testUnknownKeysAreNotReported() { + ServerConfiguration conf = new ServerConfiguration(); + conf.setProperty("brokerServicePort", "6650"); + + assertTrue(BookieServer.overriddenConfig(conf).isEmpty()); + } + + @Test + public void testValueRejectedByGetterIsReported() { + ServerConfiguration conf = new ServerConfiguration(); + conf.setProperty("ledgerManagerFactoryClass", "does.not.Exist"); + + Map overrides = BookieServer.overriddenConfig(conf); + + assertEquals("does.not.Exist", overrides.get("ledgerManagerFactoryClass")); + } +}