chihsuan commented on code in PR #10926:
URL: https://github.com/apache/ozone/pull/10926#discussion_r4017132068
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/conf/OzoneConfiguration.java:
##########
@@ -446,6 +452,38 @@ public synchronized void reloadConfiguration() {
delegatingProps = null;
}
+ /**
+ * Sets {@code value} unless the property was already set explicitly
+ * (programmatically, from the command line, from a {@code *-site.xml}, or
from a
+ * user-provided resource). Values that come only from Ozone's built-in
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);
Review Comment:
Could we check whether the property currently has a value before inspecting
its sources? Since `unset()` removes the value but leaves the source metadata,
I don't think it works if we call `set → unset → setIfUnset`.
##########
hadoop-ozone/integration-test/src/test/java/org/apache/ozone/test/ClusterForTests.java:
##########
@@ -61,9 +60,7 @@ protected static OzoneConfiguration createBaseConfiguration()
{
raftClientConfig.setRpcWatchRequestTimeout(Duration.ofSeconds(10));
conf.setFromObject(raftClientConfig);
- OzoneClientConfig clientConfig = conf.getObject(OzoneClientConfig.class);
- clientConfig.setStreamBufferFlushDelay(false);
- conf.setFromObject(clientConfig);
+ conf.setBoolean("ozone.client.stream.buffer.flush.delay", false);
Review Comment:
Could we apply the same change to the other tests with this pattern? It
looks like there are five more that round-trip `OzoneClientConfig` before
`newBuilder`:
- TestXceiverClientManagerSC
- TestClientRetryContainerStateMachineFailures
- TestContainerStateMachine
- TestContainerStateMachineFailures
- TestValidateBCSIDOnRestart
##########
hadoop-ozone/mini-cluster/src/main/java/org/apache/hadoop/ozone/ClientConfigForTesting.java:
##########
@@ -93,6 +93,24 @@ public ClientConfigForTesting setDataStreamWindowSize(long
size) {
}
public void applyTo(MutableConfigurationSource conf) {
+ applyTo(conf, false);
+ }
+
+ public void applyTo(MutableConfigurationSource conf, boolean onlyIfUnset) {
+ calculateUndefinedValues();
+
+ final MutableConfigurationSource target =
+ onlyIfUnset ? MutableConfigurationSource.ifUnsetWrapper(conf) : conf;
+ target.setFromObject(getClientConfig(conf));
Review Comment:
I wonder if a partial override could break here. For example, an explicit 4
MB stream buffer is preserved, but the flush size is reduced to 1 MB, causing
`OzoneClientConfig` validation to fail and preventing client initialization.
Would it make sense to leave all three buffer settings unchanged when any one
is explicitly set?
--
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]
