From 41f85bef96f14a92bd01ba5f087da583469ed606 Mon Sep 17 00:00:00 2001 From: Matteo Merli Date: Wed, 16 Sep 2026 16:03:28 -0700 Subject: [PATCH] Log only the bookie settings that differ from the built-in defaults The "Starting Bookie server" log line introduced in #4768 was meant to print only the overridden settings, but it compared every key against an empty ServerConfiguration. The defaults are not stored in the configuration object, they are hard-coded inside the getters, so the lookup returned null for every key and all of them were reported as overrides. When the configuration is shared with other components, for example Pulsar standalone loading the broker settings into the same properties, hundreds of unrelated entries were printed. Compute the overrides by invoking every ServerConfiguration getter on both the actual configuration and an empty one, through a subclass that records the property keys each getter reads. When the two results differ, the recorded keys that are explicitly set are reported with their raw values. Keys that no getter reads are left out. --- .../apache/bookkeeper/proto/BookieServer.java | 100 +++++++++++++++--- .../BookieServerOverriddenConfigTest.java | 98 +++++++++++++++++ 2 files changed, 184 insertions(+), 14 deletions(-) create mode 100644 bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieServerOverriddenConfigTest.java 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")); + } +}