merlimat commented on code in PR #4892:
URL: https://github.com/apache/bookkeeper/pull/4892#discussion_r4288675456
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java:
##########
@@ -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.
+ *
+ * <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. Keys that no getter reads (settings of other components,
or unrelated entries loaded
+ * from a shared properties file) are not reported.
*/
- 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, 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) {
Review Comment:
Added a note to the overriddenConfig() Javadoc that the ServerConfiguration
getters must remain side-effect free and cheap, since they are all invoked
reflectively on each bookie start (56ce6807c9).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
