lhotari commented on code in PR #4892:
URL: https://github.com/apache/bookkeeper/pull/4892#discussion_r4288644050
##########
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:
**[QUALITY] invoking every public getter reflectively is fragile**
Minor, non-blocking: this relies on every public `get*`/`is*` getter of
`ServerConfiguration` being side-effect free and cheap, and on the getter set
not changing under it. Currently the class-loading getters (e.g.
`getLedgerManagerFactoryClass`) are the only heavier ones, and a throw is
handled here, so it works today. A short note in the Javadoc that getters must
stay side-effect free would help future contributors.
--
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]
