This is an automated email from the ASF dual-hosted git repository. merlimat pushed a commit to branch branch-4.18 in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
commit eddbb56fb20ee33f01b83b5e58d6bfd1487f7bfe Author: Matteo Merli <[email protected]> AuthorDate: Sat Oct 17 05:34:46 2026 -0700 Log only the bookie settings that differ from the built-in defaults (#4892) --- .../org/apache/bookkeeper/proto/BookieServer.java | 108 +++++++++++++++++--- .../proto/BookieServerOverriddenConfigTest.java | 110 +++++++++++++++++++++ 2 files changed, 204 insertions(+), 14 deletions(-) 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 2fa5e90e0b..b835798eef 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,11 +27,18 @@ import com.google.common.annotations.VisibleForTesting; 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 java.util.regex.Pattern; import lombok.CustomLog; import org.apache.bookkeeper.bookie.Bookie; import org.apache.bookkeeper.bookie.BookieCriticalThread; @@ -39,6 +46,7 @@ import org.apache.bookkeeper.bookie.BookieException; 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; @@ -76,6 +84,9 @@ public class BookieServer { // Exception handler private volatile UncaughtExceptionHandler uncaughtExceptionHandler = null; + // configuration keys whose values must not be logged + private static final Pattern SENSITIVE_KEY = Pattern.compile("password|secret|token", Pattern.CASE_INSENSITIVE); + public BookieServer(ServerConfiguration conf, Bookie bookie, StatsLogger statsLogger, @@ -220,28 +231,97 @@ public class BookieServer { } /** - * 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. + * + * <p>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, with the value masked when the key looks like a secret. Keys that no getter reads + * (settings of other components, or unrelated entries loaded from a shared properties file) are not + * reported. + * + * <p>Since every public {@code get*}/{@code is*} getter of {@link ServerConfiguration} is invoked + * reflectively here on each bookie start, those getters must remain side-effect free and cheap. */ - private static Map<String, Object> overriddenConfig(ServerConfiguration conf) { - ServerConfiguration defaults = new ServerConfiguration(); + @VisibleForTesting + static Map<String, Object> overriddenConfig(ServerConfiguration conf) { + KeyRecordingConfiguration actual = new KeyRecordingConfiguration(conf); + KeyRecordingConfiguration defaults = new KeyRecordingConfiguration(new ServerConfiguration()); Map<String, Object> overrides = new TreeMap<>(); - Iterator<String> 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<String> keys = new HashSet<>(actual.keys); + keys.addAll(defaults.keys); + for (String key : keys) { + if (conf.containsKey(key)) { + overrides.put(key, SENSITIVE_KEY.matcher(key).find() ? "******" : 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. + * + * <p>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<String> 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<Object> 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 0000000000..bb6864e1d6 --- /dev/null +++ b/bookkeeper-server/src/test/java/org/apache/bookkeeper/proto/BookieServerOverriddenConfigTest.java @@ -0,0 +1,110 @@ +/* + * + * 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.assertFalse; +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<String, Object> 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<String, Object> 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 testSensitiveValuesAreMasked() { + ServerConfiguration conf = new ServerConfiguration(); + conf.setHttpServerKeyStorePassword("hunter2"); + + Map<String, Object> overrides = BookieServer.overriddenConfig(conf); + + assertEquals("******", overrides.get("httpServerKeyStorePassword")); + assertFalse(overrides.toString().contains("hunter2")); + } + + @Test + public void testValueRejectedByGetterIsReported() { + ServerConfiguration conf = new ServerConfiguration(); + conf.setProperty("ledgerManagerFactoryClass", "does.not.Exist"); + + Map<String, Object> overrides = BookieServer.overriddenConfig(conf); + + assertEquals("does.not.Exist", overrides.get("ledgerManagerFactoryClass")); + } +}
