Copilot commented on code in PR #10926:
URL: https://github.com/apache/ozone/pull/10926#discussion_r3695326068
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/conf/OzoneConfiguration.java:
##########
@@ -446,6 +446,36 @@ public synchronized void reloadConfiguration() {
delegatingProps = null;
}
+ /**
+ * Sets {@code value} unless the property was already set explicitly
+ * (programmatically, from the command line, or from a {@code *-site.xml}).
+ * Values that come only from default resources ({@code *-default.xml}) are
overridden.
+ * <p>
+ * Hadoop {@link Configuration#setIfUnset(String, String)} uses {@code
get(name) == null},
+ * which never succeeds for keys present in default resources after
HDDS-12777.
+ */
+ @Override
+ public synchronized void setIfUnset(String name, String value) {
+ if (!isExplicitlySet(name)) {
+ set(name, value);
+ }
+ }
+
+ private boolean isExplicitlySet(String name) {
+ String[] sources = getPropertySources(name);
+ if (sources == null) {
+ return false;
+ }
+ for (String source : sources) {
+ // Any source other than a *-default.xml (programmatically, command line,
+ // a *-site.xml, or a custom resource) counts as explicitly set.
+ if (source != null && !source.endsWith("-default.xml")) {
+ return true;
+ }
+ }
+ return false;
+ }
Review Comment:
isExplicitlySet() treats any property source ending with "-default.xml" as
non-explicit. This conflicts with the method comment that says values from a
custom resource must be preserved: a user-provided resource named
"custom-default.xml" would be incorrectly treated as a default and get
overridden by setIfUnset(). Consider restricting the "default" check to Ozone's
known built-in *-default.xml resources.
##########
hadoop-hdds/config/src/main/java/org/apache/hadoop/hdds/conf/MutableConfigurationSource.java:
##########
@@ -17,9 +17,66 @@
package org.apache.hadoop.hdds.conf;
+import java.io.IOException;
+import java.util.Collection;
+
/**
* Configuration that can be both read and written.
*/
public interface MutableConfigurationSource
extends ConfigurationSource, ConfigurationTarget {
+
+ /**
+ * Sets {@code value} for {@code key} only if the key is not already set.
+ * Default implementation treats any non-null {@link #get(String)} result as
set.
+ * {@code OzoneConfiguration} (in hdds-common) overrides this to allow
+ * overriding values that come only from default resources.
+ */
+ default void setIfUnset(String key, String value) {
+ if (get(key) == null) {
+ set(key, value);
+ }
+ }
+
+ /**
+ * Creates a wrapper config that changes {@link #set(String, String)} to
+ * {@link #setIfUnset(String, String)}. In other words, value is stored only
if
+ * no existing value is explicitly set.
+ */
+ static MutableConfigurationSource ifUnsetWrapper(MutableConfigurationSource
wrapped) {
+ return new IfUnsetWrapper(wrapped);
+ }
+
+ /**
+ * Delegates all calls to another configuration object, but changes
semantics of
+ * {@link #set(String, String)} to {@link #setIfUnset(String, String)}.
+ */
+ class IfUnsetWrapper implements MutableConfigurationSource {
+
+ private final MutableConfigurationSource wrapped;
+
+ private IfUnsetWrapper(MutableConfigurationSource wrapped) {
+ this.wrapped = wrapped;
+ }
+
+ @Override
+ public String get(String key) {
+ return wrapped.get(key);
+ }
+
+ @Override
+ public Collection<String> getConfigKeys() {
+ return wrapped.getConfigKeys();
+ }
+
+ @Override
+ public char[] getPassword(String key) throws IOException {
+ return wrapped.getPassword(key);
+ }
+
+ @Override
+ public void set(String key, String value) {
+ wrapped.setIfUnset(key, value);
+ }
+ }
Review Comment:
IfUnsetWrapper changes set(key,value) to delegate to
wrapped.setIfUnset(...), but calling setIfUnset(...) on the wrapper itself
still uses the interface default implementation (get(key) == null). That makes
the wrapper internally inconsistent and can break callers that expect the
wrapper to behave the same for both set(...) and setIfUnset(...).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
