You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "henrybear327 (via GitHub)" <gi...@apache.org> on 2026/08/03 19:12:50 UTC
[PR] HDDS-15986. Report ozone local startup failures [ozone]
henrybear327 opened a new pull request, #10934:
URL: https://github.com/apache/ozone/pull/10934
## What changes were proposed in this pull request?
This PR improves the error reporting and handling.
`ozone local run` swallowed every failure it hit: shutdown and start() rollback errors went to IOUtils.closeQuietly with a null logger, the readiness wait collapsed its conditions into a single boolean, and the CLI converters rejected a value without naming it.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-15986
## How was this patch tested?
TBD
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on PR #10934:
URL: https://github.com/apache/ozone/pull/10934#issuecomment-5536473685
@henrybear327 please help to fix the https://github.com/apache/ozone/actions/runs/33605074111/job/100883379964?pr=10934, thanks!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on PR #10934:
URL: https://github.com/apache/ozone/pull/10934#issuecomment-5196034406
@peterxcli PTAL, thanks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894148521
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -643,30 +780,64 @@ private void startDatanodes(List<OzoneConfiguration> datanodeConfigurations) {
}
private void waitForClusterReadiness(Duration timeout) throws Exception {
+ waitForReadiness(this::clusterReadinessBlocker, "Ozone cluster", timeout);
+ }
+
+ /**
+ * Polls {@code blocker} until it reports ready. {@code blocker} returns why {@code subject} is
+ * not ready yet, so the wait can name the unmet condition instead of reporting a bare timeout
+ * that cannot distinguish a slow start from a stuck one.
+ */
+ static void waitForReadiness(Supplier<String> blocker, String subject, Duration timeout)
+ throws InterruptedException, TimeoutException {
long deadlineNanos = System.nanoTime() + timeout.toNanos();
+ long nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
while (true) {
- if (isClusterReady()) {
+ String reason = blocker.get();
+ if (reason == null) {
return;
}
if (System.nanoTime() >= deadlineNanos) {
- throw new TimeoutException("Timed out waiting " + timeout
- + " for the local Ozone cluster to become ready.");
+ throw new TimeoutException("Timed out waiting " + timeout + " for the local " + subject
+ + " to become ready: " + reason + ".");
+ }
+ if (System.nanoTime() >= nextLogNanos) {
+ LOG.info("Waiting for the local {} to become ready: {}.", subject, reason);
+ nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
}
Thread.sleep(READINESS_POLL_INTERVAL_MILLIS);
}
}
- private boolean isClusterReady() {
- if (!scm.checkLeader() || !om.isLeaderReady()) {
- return false;
+ /**
+ * Returns why the cluster is not usable yet, or null once it is ready. The cluster is usable
+ * once SCM and OM are leader-ready, every datanode has registered with SCM, and SCM has left
+ * safe mode.
+ */
+ private String clusterReadinessBlocker() {
+ if (!scm.checkLeader()) {
+ return "SCM has no Ratis leader yet";
+ }
+ if (!om.isLeaderReady()) {
+ return "OM is not leader-ready yet";
}
- if (config.getDatanodes() == 0) {
- return true;
+ int registered = scm.getScmNodeManager().getAllNodes().size();
+ if (registered < config.getDatanodes()) {
+ return "only " + registered + " of " + config.getDatanodes()
+ + " datanodes have registered with SCM";
}
- // The cluster is usable once every datanode has registered with SCM and
- // SCM has left safe mode.
- return scm.getScmNodeManager().getAllNodes().size() >= config.getDatanodes()
- && !scm.isInSafeMode();
+ // Registration alone is not enough: SCM refuses block allocation until it leaves safe mode.
+ if (scm.isInSafeMode()) {
+ return "SCM is still in safe mode (" + unmetSafeModeRules() + ")";
+ }
+ return null;
+ }
+
+ private String unmetSafeModeRules() {
+ return scm.getScmSafeModeManager().getRuleStatus().entrySet().stream()
Review Comment:
Fixed. Sorry for uploading a version that actually broke the build.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "peterxcli (via GitHub)" <gi...@apache.org>.
peterxcli commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3880703467
##########
hadoop-ozone/tools/pom.xml:
##########
@@ -46,6 +46,10 @@
<groupId>jakarta.xml.bind</groupId>
<artifactId>jakarta.xml.bind-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
Review Comment:
This entry only exists to put `org.apache.commons.lang3.tuple.Pair` on the compile classpath for the `getLeft()`/`getRight()` calls in `unmetSafeModeRules()`. Once that helper is rewritten against `SafeModeRuleStatusProto`, `commons-lang3` has no consumer in the module.
That matters because the root pom binds `maven-dependency-plugin:analyze-only` with `failOnWarning=true` to every module, so a Java-only fix fails the build a second time:
```
[ERROR] Used undeclared dependencies found:
[ERROR] org.apache.ozone:hdds-interface-admin
[ERROR] Unused declared dependencies found:
[ERROR] org.apache.commons:commons-lang3
```
Swapping this entry for `org.apache.ozone:hdds-interface-admin` gives `BUILD SUCCESS` on `mvn verify` — I confirmed both directions locally.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
Review Comment:
Small inconsistency rather than a bug.
`conf.getTimeDuration(key, default, MILLISECONDS)` does exactly what the new `--startup-timeout` guard in `OzoneLocal` exists to prevent: it accepts a unitless value and assumes the unit. I confirmed `hdds.heartbeat.interval=1000` in a user's config parses as 1000 ms and is therefore accepted as satisfying the required `1s`.
Harmless here since the two agree, but the PR takes opposite positions on the same ambiguity in two files — rejected on the CLI, silently assumed from config.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ /**
+ * Applies a replication factor the local runtime requires, reading the configured value the way
+ * {@link org.apache.hadoop.hdds.client.ReplicationConfig#parse} does, which accepts both the
+ * numeric and the named spelling.
+ *
+ * @throws IOException if the user configured {@code key} with a different factor
+ */
+ private void setLocalOverrideReplication(OzoneConfiguration conf, String key,
+ ReplicationFactor value) throws IOException {
+ String configured = conf.get(key);
+ if (configured != null && parseReplicationFactor(configured) != value) {
+ rejectUserConfigured(conf, key, value.name());
+ }
+ conf.set(key, value.name());
+ }
+
+ /**
+ * Throws when the value {@code conf} carries for {@code key} is the user's choice rather than a
+ * shipped default. The message names the source because the user has to find the value to
+ * remove it.
+ */
+ private static void rejectUserConfigured(OzoneConfiguration conf, String key, String required)
+ throws IOException {
+ String source = userConfiguredSource(conf, key);
+ if (source != null) {
+ throw new IOException("ozone local requires " + key + "=" + required
Review Comment:
Two things about this message.
**1. `-D` conflicts name an internal token.** `GenericCli.setConfigurationOverrides` applies `-D` through the two-arg `Configuration.set()`, so Hadoop records the source as the literal string `programmatically`. I reproduced it:
```
ozone local requires hdds.heartbeat.interval=1s, but the configuration sets 30s
(source: programmatically). Remove that value, or run with a configuration
directory (OZONE_CONF_DIR) that does not set it.
```
`programmatically` means nothing to a CLI user, and the `OZONE_CONF_DIR` advice points somewhere unrelated to a flag they typed. This is the path `conflictingConfigReachesStderrThroughGenericCli` drives, so it's the message most likely to be seen. Worth special-casing to something like "set on the command line (`-D`/`--set`)" and dropping the second sentence for that case.
Also: every current test passes an explicit source to `seed.set(...)`, so none of them exercises the `-D` path. A test seeded with the two-arg `set()` would cover it.
**2. Minor, on the `OZONE_CONF_DIR` half.** `ozone_verify_confdir` is `[[ -f "$1/ozone-site.xml" ]]`, so pointing `OZONE_CONF_DIR` at an *empty* directory is silently discarded and falls back to `$OZONE_HOME/etc/hadoop`. Defensible as literally written — a conf dir does contain an `ozone-site.xml` — but "a configuration directory that does not set it" reads like "a directory without that setting", and the failure is silent.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -351,28 +363,25 @@ private void stopScm() {
}
}
- private void configureLocalDefaults(OzoneConfiguration conf) {
- conf.set(OZONE_METADATA_DIRS, metadataDir().toString());
+ private void configureLocalDefaults(OzoneConfiguration conf) throws IOException {
+ setLocalOverride(conf, OZONE_METADATA_DIRS, metadataDir().toString());
Review Comment:
This is the one I'd most like to see reconsidered.
The required value is `metadataDir()` — a path computed per run from `--data-dir`. No value a user could plausibly have in `ozone-site.xml` will ever equal it, so **any** user-set `ozone.metadata.dirs` aborts the run outright.
That key isn't exotic:
- tagged `REQUIRED` in `ozone-default.xml` (shipped with an empty `<value/>`, so there's no default to fall back on),
- `ozone genconf` writes it into the config it generates,
- every compose environment in this repo sets it,
- `ozone_add_classpath "${OZONE_CONF_DIR}" before` puts that `ozone-site.xml` on the classpath, so the value is attributed to the user.
A pristine tarball is fine (the shipped `etc/hadoop/ozone-site.xml` is empty). The people who break are developers who also run a real cluster — much of the audience for a "just run Ozone locally" command.
Suggestion: keep plain `conf.set()` for keys whose required value is a path the runtime generates and owns, and reserve `setLocalOverride` for keys where a conflicting user value would actually change observable cluster behaviour.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ /**
+ * Applies a replication factor the local runtime requires, reading the configured value the way
+ * {@link org.apache.hadoop.hdds.client.ReplicationConfig#parse} does, which accepts both the
+ * numeric and the named spelling.
+ *
+ * @throws IOException if the user configured {@code key} with a different factor
+ */
+ private void setLocalOverrideReplication(OzoneConfiguration conf, String key,
+ ReplicationFactor value) throws IOException {
+ String configured = conf.get(key);
+ if (configured != null && parseReplicationFactor(configured) != value) {
+ rejectUserConfigured(conf, key, value.name());
+ }
+ conf.set(key, value.name());
+ }
+
+ /**
+ * Throws when the value {@code conf} carries for {@code key} is the user's choice rather than a
+ * shipped default. The message names the source because the user has to find the value to
+ * remove it.
+ */
+ private static void rejectUserConfigured(OzoneConfiguration conf, String key, String required)
+ throws IOException {
+ String source = userConfiguredSource(conf, key);
+ if (source != null) {
+ throw new IOException("ozone local requires " + key + "=" + required
+ + ", but the configuration sets " + conf.get(key) + " (source: " + source
+ + "). Remove that value, or run with a configuration directory (OZONE_CONF_DIR)"
+ + " that does not set it.");
+ }
+ }
+
+ private static boolean matchesInt(OzoneConfiguration conf, String key, int value) {
+ try {
+ return conf.getInt(key, value) == value;
+ } catch (NumberFormatException unreadable) {
+ // A value the accessor cannot read is a conflict; the caller reports it by key.
+ return false;
+ }
+ }
+
+ private static boolean matchesDuration(OzoneConfiguration conf, String key,
+ long requiredMillis) {
+ try {
+ return conf.getTimeDuration(key, requiredMillis, TimeUnit.MILLISECONDS) == requiredMillis;
+ } catch (NumberFormatException unreadable) {
+ return false;
+ }
+ }
+
+ /** Returns the factor {@code value} names in either spelling, or null if it names neither. */
+ private static ReplicationFactor parseReplicationFactor(String value) {
+ String trimmed = value.trim();
+ try {
+ return ReplicationFactor.valueOf(Integer.parseInt(trimmed));
+ } catch (IllegalArgumentException notNumeric) {
+ try {
+ return ReplicationFactor.valueOf(trimmed);
+ } catch (IllegalArgumentException notNamed) {
+ return null;
+ }
+ }
+ }
+
+ /**
+ * Returns where {@code key} got the value the user chose, or null if the user chose none. A
+ * value whose last source is the shipped ozone-default.xml is a default, not a user choice.
+ * The comparison is exact: Configuration records a classpath resource by its bare name, so a
+ * file the user named with {@code --conf} keeps its path here and stays a user choice however
+ * that file is called.
+ */
+ private static String userConfiguredSource(OzoneConfiguration conf, String key) {
+ String[] sources = conf.getPropertySources(key);
+ if (sources == null || sources.length == 0) {
+ return null;
+ }
+ String source = sources[sources.length - 1];
+ return OZONE_DEFAULT_XML.equals(source) ? null : source;
Review Comment:
Forward-looking note — **this is correct today**, I probed a fresh `OzoneConfiguration` and none of the eleven guarded keys lives in a generated resource.
`OzoneConfiguration.activate()` registers thirteen default resources, including eight `<module>-default.xml` files generated from `@Config` annotations and shipped inside the jars exactly like `ozone-default.xml`. This check recognises one of them.
Why it's worth hardening: eight of the eleven guarded keys *do* have a shipped default that conflicts with the required value (`ozone.server.default.replication=3`, `hdds.heartbeat.interval=30s`, `hdds.scm.safemode.min.datanode=3`, ...). For those, this single string comparison is the only thing standing between the shipped defaults and an abort on every run. Migrating any one of them to a `@Config` annotation — which the project is doing elsewhere — would silently break the command.
Deriving the set from `OzoneConfiguration.getConfigurationResourceFiles()` would remove the trap, and widening `shippedDefaultIsNotTreatedAsUserConfig` to cover every guarded key would catch it if it ever regressed.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -643,30 +780,64 @@ private void startDatanodes(List<OzoneConfiguration> datanodeConfigurations) {
}
private void waitForClusterReadiness(Duration timeout) throws Exception {
+ waitForReadiness(this::clusterReadinessBlocker, "Ozone cluster", timeout);
+ }
+
+ /**
+ * Polls {@code blocker} until it reports ready. {@code blocker} returns why {@code subject} is
+ * not ready yet, so the wait can name the unmet condition instead of reporting a bare timeout
+ * that cannot distinguish a slow start from a stuck one.
+ */
+ static void waitForReadiness(Supplier<String> blocker, String subject, Duration timeout)
+ throws InterruptedException, TimeoutException {
long deadlineNanos = System.nanoTime() + timeout.toNanos();
+ long nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
while (true) {
- if (isClusterReady()) {
+ String reason = blocker.get();
+ if (reason == null) {
return;
}
if (System.nanoTime() >= deadlineNanos) {
- throw new TimeoutException("Timed out waiting " + timeout
- + " for the local Ozone cluster to become ready.");
+ throw new TimeoutException("Timed out waiting " + timeout + " for the local " + subject
+ + " to become ready: " + reason + ".");
+ }
+ if (System.nanoTime() >= nextLogNanos) {
+ LOG.info("Waiting for the local {} to become ready: {}.", subject, reason);
+ nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
}
Thread.sleep(READINESS_POLL_INTERVAL_MILLIS);
}
}
- private boolean isClusterReady() {
- if (!scm.checkLeader() || !om.isLeaderReady()) {
- return false;
+ /**
+ * Returns why the cluster is not usable yet, or null once it is ready. The cluster is usable
+ * once SCM and OM are leader-ready, every datanode has registered with SCM, and SCM has left
+ * safe mode.
+ */
+ private String clusterReadinessBlocker() {
+ if (!scm.checkLeader()) {
+ return "SCM has no Ratis leader yet";
+ }
+ if (!om.isLeaderReady()) {
+ return "OM is not leader-ready yet";
}
- if (config.getDatanodes() == 0) {
- return true;
+ int registered = scm.getScmNodeManager().getAllNodes().size();
+ if (registered < config.getDatanodes()) {
+ return "only " + registered + " of " + config.getDatanodes()
+ + " datanodes have registered with SCM";
}
- // The cluster is usable once every datanode has registered with SCM and
- // SCM has left safe mode.
- return scm.getScmNodeManager().getAllNodes().size() >= config.getDatanodes()
- && !scm.isInSafeMode();
+ // Registration alone is not enough: SCM refuses block allocation until it leaves safe mode.
+ if (scm.isInSafeMode()) {
+ return "SCM is still in safe mode (" + unmetSafeModeRules() + ")";
+ }
+ return null;
+ }
+
+ private String unmetSafeModeRules() {
+ return scm.getScmSafeModeManager().getRuleStatus().entrySet().stream()
Review Comment:
**Blocker — this does not compile.**
`getRuleStatus()` returns `List<SafeModeRuleStatusProto>` on the merged master (changed by HDDS-16130 / `9e7ba24913`), and `List` has no `entrySet()`. The `getLeft()`/`getRight()` calls below were written against the old `Map<String, Pair<..>>` shape.
```
[ERROR] LocalOzoneCluster.java:[837,55] cannot find symbol
symbol: method entrySet()
location: interface java.util.List<...SafeModeRuleStatusProto>
```
The replacement accessors are on the proto: `getRuleName()`, `getValidate()`, `getStatusText()`. Also worth noting `getScmSafeModeManager()` is `@VisibleForTesting` — `StorageContainerManager.getRuleStatus()` is the public accessor for exactly this list:
```java
private String unmetSafeModeRules() {
return scm.getRuleStatus().stream()
.filter(rule -> !rule.getValidate())
.map(rule -> rule.getRuleName() + ": " + rule.getStatusText())
.collect(Collectors.joining("; "));
}
```
See the comment on `pom.xml` — the fix has a second half.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/OzoneLocal.java:
##########
@@ -197,13 +197,32 @@ static class RunCommand extends AbstractSubcommand implements Callable<Void> {
public Void call() throws Exception {
LocalOzoneClusterConfig config = resolveConfig();
try (LocalOzoneRuntime runtime = createRuntime(config, getOzoneConf())) {
- runtime.start();
+ start(runtime);
printSummary(runtime, config);
awaitShutdown(runtime);
}
return null;
}
+ /**
+ * Starts {@code runtime}, naming the ways to get more detail before letting the failure
+ * through. Service logging is off by default for this command, so the detail goes to the log
+ * for {@code --loglevel INFO}; the failure itself is rethrown unchanged so
+ * {@link GenericCli#printError} prints its own message rather than a restatement of it.
+ */
+ private void start(LocalOzoneRuntime runtime) throws Exception {
Review Comment:
Two small things about the hint.
**Ordering.** It goes to `err()` and then rethrows, so `GenericCli#printError` prints the actual reason afterwards — your own test pins that (`lines[0]` is the hint, `lines[1]` is the rejection). The user reads the remedy before knowing what went wrong. Overriding `printError` in `OzoneLocal` to call `super` first and append the hint would put the reason on top.
**It's state-blind.** Both halves are testable right here — `isVerbose()` is inherited from `AbstractSubcommand`, and `LOG.isInfoEnabled()` answers the other half — so someone already running `--loglevel INFO --verbose` is told to add `--loglevel INFO` and `--verbose`, and gets the stack trace twice (once from the `LOG.error` above through the console appender, once from `printError`). Gating each clause on the state it names would also remove the duplicate trace.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -643,30 +780,64 @@ private void startDatanodes(List<OzoneConfiguration> datanodeConfigurations) {
}
private void waitForClusterReadiness(Duration timeout) throws Exception {
+ waitForReadiness(this::clusterReadinessBlocker, "Ozone cluster", timeout);
+ }
+
+ /**
+ * Polls {@code blocker} until it reports ready. {@code blocker} returns why {@code subject} is
+ * not ready yet, so the wait can name the unmet condition instead of reporting a bare timeout
+ * that cannot distinguish a slow start from a stuck one.
+ */
+ static void waitForReadiness(Supplier<String> blocker, String subject, Duration timeout)
+ throws InterruptedException, TimeoutException {
long deadlineNanos = System.nanoTime() + timeout.toNanos();
+ long nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
while (true) {
- if (isClusterReady()) {
+ String reason = blocker.get();
+ if (reason == null) {
return;
}
if (System.nanoTime() >= deadlineNanos) {
- throw new TimeoutException("Timed out waiting " + timeout
- + " for the local Ozone cluster to become ready.");
+ throw new TimeoutException("Timed out waiting " + timeout + " for the local " + subject
+ + " to become ready: " + reason + ".");
+ }
+ if (System.nanoTime() >= nextLogNanos) {
+ LOG.info("Waiting for the local {} to become ready: {}.", subject, reason);
+ nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
}
Thread.sleep(READINESS_POLL_INTERVAL_MILLIS);
}
}
- private boolean isClusterReady() {
- if (!scm.checkLeader() || !om.isLeaderReady()) {
- return false;
+ /**
+ * Returns why the cluster is not usable yet, or null once it is ready. The cluster is usable
+ * once SCM and OM are leader-ready, every datanode has registered with SCM, and SCM has left
+ * safe mode.
+ */
+ private String clusterReadinessBlocker() {
+ if (!scm.checkLeader()) {
+ return "SCM has no Ratis leader yet";
+ }
+ if (!om.isLeaderReady()) {
+ return "OM is not leader-ready yet";
}
- if (config.getDatanodes() == 0) {
- return true;
+ int registered = scm.getScmNodeManager().getAllNodes().size();
+ if (registered < config.getDatanodes()) {
+ return "only " + registered + " of " + config.getDatanodes()
+ + " datanodes have registered with SCM";
}
- // The cluster is usable once every datanode has registered with SCM and
- // SCM has left safe mode.
- return scm.getScmNodeManager().getAllNodes().size() >= config.getDatanodes()
- && !scm.isInSafeMode();
+ // Registration alone is not enough: SCM refuses block allocation until it leaves safe mode.
+ if (scm.isInSafeMode()) {
Review Comment:
After the compile fix, this can render an empty reason.
`scm.isInSafeMode()` reads the cached status, while `unmetSafeModeRules()` filters on each rule's live `validate()`. Those aren't updated together — the status only flips inside `validateSafeModeExitRules` once every rule has been recorded. In the window where all rules validate but the status hasn't flipped, `joining("; ")` over an empty stream gives `""`:
```
Timed out waiting PT2M for the local Ozone cluster to become ready:
SCM is still in safe mode ().
```
Which is the "bare timeout that cannot distinguish a slow start from a stuck one" the `waitForReadiness` javadoc says it's fixing. Falling back to listing all rules with their status text when nothing reports unmet would close it.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -693,6 +883,7 @@ private void prepareStorageLayout() throws IOException {
switch (config.getFormatMode()) {
case ALWAYS:
+ LOG.info("Removing local Ozone data dir {} (format mode ALWAYS).", dataDir);
Review Comment:
This is a real improvement over master (which printed nothing here), but it won't reach the user on a default run.
`ozone local` dispatches with `OZONE_RUN_ARTIFACT_NAME="ozone-tools"`, and `ozone_suppress_shell_log` matches that and pins `OZONE_LOGLEVEL=OFF` / `OZONE_ROOT_LOGGER=OFF,console` unless the user already set a level. Your javadoc on `RunCommand.start()` draws exactly this conclusion for the failure hint — same reasoning applies here.
Of the new `LOG` calls this is the one I'd move: it announces the only destructive thing the command does. Every other user-facing line already goes through `out()`/`err()`.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -351,28 +363,25 @@ private void stopScm() {
}
}
- private void configureLocalDefaults(OzoneConfiguration conf) {
- conf.set(OZONE_METADATA_DIRS, metadataDir().toString());
+ private void configureLocalDefaults(OzoneConfiguration conf) throws IOException {
+ setLocalOverride(conf, OZONE_METADATA_DIRS, metadataDir().toString());
// SCM and OM share one configuration and JVM, so the Jetty base dir is a
// single service-neutral location; Jetty keeps per-context temp dirs.
conf.setIfUnset(OZONE_HTTP_BASEDIR, metadataDir() + SERVER_DIR);
- conf.set(OZONE_REPLICATION, ReplicationFactor.ONE.name());
- conf.set(OZONE_REPLICATION_TYPE, ReplicationType.STAND_ALONE.name());
- conf.set(OZONE_SERVER_DEFAULT_REPLICATION_KEY, ReplicationFactor.ONE.name());
- conf.set(OZONE_SERVER_DEFAULT_REPLICATION_TYPE_KEY,
+ setLocalOverrideReplication(conf, OZONE_REPLICATION, ReplicationFactor.ONE);
+ setLocalOverride(conf, OZONE_REPLICATION_TYPE, ReplicationType.STAND_ALONE.name());
+ setLocalOverrideReplication(conf, OZONE_SERVER_DEFAULT_REPLICATION_KEY, ReplicationFactor.ONE);
+ setLocalOverride(conf, OZONE_SERVER_DEFAULT_REPLICATION_TYPE_KEY,
ReplicationType.STAND_ALONE.name());
- conf.setBoolean(HDDS_CONTAINER_RATIS_ENABLED_KEY, false);
+ setLocalOverride(conf, HDDS_CONTAINER_RATIS_ENABLED_KEY, false);
// A single-node local cluster can heartbeat aggressively; this speeds
- // datanode registration and safe-mode exit. Use set(), not setIfUnset():
- // ozone-default.xml supplies the 30s default that would otherwise defeat
- // the override.
- conf.set(HDDS_HEARTBEAT_INTERVAL, "1s");
- conf.setBoolean(HDDS_SCM_SAFEMODE_PIPELINE_CREATION, false);
- conf.setInt(HDDS_SCM_SAFEMODE_MIN_DATANODE,
- Math.max(1, config.getDatanodes()));
- conf.setTimeDuration(OZONE_OM_RATIS_MINIMUM_TIMEOUT_KEY,
- LOCAL_RATIS_RPC_TIMEOUT_SECONDS, TimeUnit.SECONDS);
- conf.set(HDDS_SCM_WAIT_TIME_AFTER_SAFE_MODE_EXIT, "3s");
+ // datanode registration and safe-mode exit.
+ setLocalOverrideDuration(conf, HDDS_HEARTBEAT_INTERVAL, "1s");
+ setLocalOverride(conf, HDDS_SCM_SAFEMODE_PIPELINE_CREATION, false);
+ setLocalOverride(conf, HDDS_SCM_SAFEMODE_MIN_DATANODE, Math.max(1, config.getDatanodes()));
Review Comment:
With `requireSupportedDatanodeCount()` now rejecting `< 1`, this `Math.max(1, ...)` can never fire.
(To be clear, the new guard itself is worth keeping — `LocalOzoneCluster` is public and `TestLocalOzoneClusterRuntime` in `integration-test` builds one straight from `LocalOzoneClusterConfig.builder()`, bypassing `resolveConfig()`'s CLI validation. So it's a genuine class-level invariant, not dead code.) It's just that two guards now claim the same responsibility — the clamp can go.
##########
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/local/TestOzoneLocal.java:
##########
@@ -250,14 +300,35 @@ void resolveConfigRejectsDatanodeCountBelowOne() {
@Test
void resolveConfigRejectsInvalidDuration() {
- assertParseError("--startup-timeout", "forever", "--startup-timeout");
+ // Pins the value echo: picocli's wrapper already names the option, so asserting on the
+ // option alone would pass even if the converter dropped the value from its message.
+ assertParseError("--startup-timeout", "forever", "Invalid duration 'forever'");
}
@Test
void resolveConfigRejectsNonPositiveDuration() {
assertConfigError("--startup-timeout", "0s", "--startup-timeout");
}
+ @Test
+ void resolveConfigRejectsDurationWithoutTimeUnit() {
+ // Without the unit check this parses as 120 milliseconds, so the run dies with an unrelated
+ // timeout instead of telling the user the value was misread. Asserts the quoted value: the
+ // bare digits also occur in the static "like 120s" hint, which would mask a dropped echo.
+ assertParseError("--startup-timeout", "120", "Missing time unit in '120'");
+ }
+
+ @Test
+ void resolveConfigAcceptsHadoopStyleMinutes() {
+ assertEquals(Duration.ofMinutes(2), resolve("--startup-timeout", "2m")
+ .getStartupTimeout());
+ }
+
+ @Test
+ void invalidFormatModeMessageNamesOffendingValue() {
Review Comment:
This is the same option, value, and helper as `resolveConfigRejectsInvalidFormat` above — only the expected substring differs.
You handled the structurally identical duration case the other way: `resolveConfigRejectsInvalidDuration` was tightened *in place* from `"--startup-timeout"` to `"Invalid duration 'forever'"` rather than duplicated. Same treatment here would drop a method.
For what it's worth the assertion itself is sound — I compiled a probe against the pinned picocli 4.7.5 and its wrapper does **not** echo the offending value (a value-less converter message yields `Invalid value for option '--format': Expected one of: ...`), so this does pin the converter's own behaviour. Purely about where the assertion lives.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/OzoneLocal.java:
##########
@@ -213,6 +232,7 @@ private void printSummary(LocalOzoneRuntime runtime, LocalOzoneClusterConfig con
writer.println("Local Ozone is running from " + config.getDataDir());
writer.println("SCM RPC: " + runtime.getDisplayHost() + ":" + runtime.getScmPort());
writer.println("OM RPC: " + runtime.getDisplayHost() + ":" + runtime.getOmPort());
+ writer.println("Datanodes: " + config.getDatanodes());
Review Comment:
Nit, take or leave: this only runs on the success path after `start()` returned, so it isn't part of reporting startup failures.
It's also the one new user-visible line with no coverage — `runCommandStartsRuntimeAndPrintsStartupSummary` asserts the other four summary lines individually but wasn't extended. One `assertTrue(text.contains("Datanodes: 1"), text);` would close it, or it could land separately.
##########
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/local/TestLocalOzoneCluster.java:
##########
@@ -349,6 +354,226 @@ void prepareConfigurationRejectsTooManyDatanodes() throws Exception {
+ "; each datanode reserves 8 local ports.", error.getMessage());
}
+ @Test
+ void zeroDatanodesIsRejected() throws Exception {
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone"))
+ .setDatanodes(0)
+ .build();
+
+ IOException error = assertPrepareFails(config);
+
+ // configureLocalDefaults() requires one datanode for safe mode, so a zero-datanode cluster
+ // can never leave it; without this the run only fails when the readiness wait times out.
+ assertTrue(error.getMessage().contains("Datanode count 0"), error.getMessage());
+ }
+
+ @Test
+ void keyUnsetAfterBeingConfiguredIsNotRejected() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // Configuration#unset() drops the value but keeps the entry in updatingResource, so the key
+ // still reports a source with no value behind it.
+ seed.set(OZONE_METADATA_DIRS, "/somewhere/else", "test-ozone-site.xml");
+ seed.unset(OZONE_METADATA_DIRS);
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertNotNull(cluster.prepareConfiguration().getConfiguration().get(OZONE_METADATA_DIRS));
+ }
+ }
+
+ @Test
+ void defaultsFileNamedByPathCountsAsUserConfig() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // Only the shipped classpath resource is a default. A file the user pointed at with --conf
+ // is their choice however it is named.
+ seed.set(OZONE_REPLICATION, ReplicationFactor.THREE.name(), "/home/me/ozone-default.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ assertTrue(error.getMessage().contains(OZONE_REPLICATION), error.getMessage());
+ }
+
+ @Test
+ void tooManyDatanodesIsRejectedBeforeFormatDeletesDataDir() throws Exception {
+ Path dataDir = tempDir.resolve("local-ozone");
+ Path marker = writeMarker(dataDir, "keep me");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(dataDir)
+ .setFormatMode(LocalOzoneClusterConfig.FormatMode.ALWAYS)
+ .setDatanodes(LocalOzoneCluster.MAX_DATANODES + 1)
+ .build();
+
+ assertPrepareFails(config);
+
+ assertTrue(Files.exists(marker),
+ "format ALWAYS must not delete the data dir for a run that cannot start");
+ }
+
+ @Test
+ void conflictingUserConfigIsRejected() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(OZONE_REPLICATION, ReplicationFactor.THREE.name(), "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ // The user has to locate the value to remove it, so the message carries the key, the value
+ // ozone local requires, the configured value, and the source Configuration recorded.
+ String message = error.getMessage();
+ assertTrue(message.contains(OZONE_REPLICATION), message);
+ assertTrue(message.contains(ReplicationFactor.ONE.name()), message);
+ assertTrue(message.contains(ReplicationFactor.THREE.name()), message);
+ assertTrue(message.contains("test-ozone-site.xml"), message);
+ }
+
+ @Test
+ void userConfigMatchingTheLocalRequirementIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(OZONE_REPLICATION, ReplicationFactor.ONE.name(), "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertEquals(ReplicationFactor.ONE.name(),
+ cluster.prepareConfiguration().getConfiguration().get(OZONE_REPLICATION));
+ }
+ }
+
+ @Test
+ void conflictingUserConfigIsRejectedBeforeFormatDeletesDataDir() throws Exception {
+ Path dataDir = tempDir.resolve("local-ozone");
+ Path marker = writeMarker(dataDir, "keep me");
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(OZONE_REPLICATION, ReplicationFactor.THREE.name(), "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(dataDir)
+ .setFormatMode(LocalOzoneClusterConfig.FormatMode.ALWAYS)
+ .build();
+
+ assertPrepareFails(config, seed);
+
+ assertTrue(Files.exists(marker),
+ "format ALWAYS must not delete the data dir for a run that cannot start");
+ }
+
+ @Test
+ void equivalentDurationSpellingIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // Same interval the local runtime requires, written in another unit. Rejecting it would refuse
+ // to start over a value that means exactly what the runtime asked for.
+ seed.set(HDDS_HEARTBEAT_INTERVAL, "1000ms", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertEquals("1s", cluster.prepareConfiguration().getConfiguration()
+ .get(HDDS_HEARTBEAT_INTERVAL));
+ }
+ }
+
+ @Test
+ void equivalentBooleanSpellingIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(HDDS_CONTAINER_RATIS_ENABLED_KEY, "FALSE", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertFalse(cluster.prepareConfiguration().getConfiguration()
+ .getBoolean(HDDS_CONTAINER_RATIS_ENABLED_KEY, true));
+ }
+ }
+
+ @Test
+ void numericReplicationIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // ReplicationConfig.parse() reads both "1" and "ONE", and compose environments in this repo
+ // write the numeric form, so it has to be accepted as the value the runtime requires.
+ seed.set(OZONE_SERVER_DEFAULT_REPLICATION_KEY, "1", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertEquals(ReplicationFactor.ONE.name(), cluster.prepareConfiguration()
+ .getConfiguration().get(OZONE_SERVER_DEFAULT_REPLICATION_KEY));
+ }
+ }
+
+ @Test
+ void conflictingDurationIsRejected() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(HDDS_HEARTBEAT_INTERVAL, "30s", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ // Comparing durations by value must not swallow a genuine conflict.
+ String message = error.getMessage();
+ assertTrue(message.contains(HDDS_HEARTBEAT_INTERVAL), message);
+ assertTrue(message.contains("30s"), message);
+ }
+
+ @Test
+ void unparseableValueIsRejected() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(HDDS_HEARTBEAT_INTERVAL, "banana", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ // A value the accessor cannot parse is a conflict, reported by the same message rather than
+ // escaping as a NumberFormatException from the comparison itself.
+ String message = error.getMessage();
+ assertTrue(message.contains(HDDS_HEARTBEAT_INTERVAL), message);
+ assertTrue(message.contains("banana"), message);
+ }
+
+ /**
+ * Regression guard: ozone-default.xml ships a value for most keys the local runtime requires
+ * (hdds.heartbeat.interval=30s, and so on) and is always on the classpath, so counting a shipped
+ * default as a user choice would reject every run.
+ */
+ @Test
+ void shippedDefaultIsNotTreatedAsUserConfig() throws Exception {
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = newCluster(config)) {
+ OzoneConfiguration prepared = cluster.prepareConfiguration().getConfiguration();
+
+ assertEquals("1s", prepared.get(HDDS_HEARTBEAT_INTERVAL));
+ assertNotEquals("1s", new OzoneConfiguration().get(HDDS_HEARTBEAT_INTERVAL),
+ "the shipped default must differ from the local value, or this guards nothing");
+ }
+ }
+
+ @Test
+ void readinessTimeoutNamesTheUnmetCondition() {
+ TimeoutException error = assertThrows(TimeoutException.class,
+ () -> LocalOzoneCluster.waitForReadiness(() -> "only 1 of 3 datanodes have registered",
Review Comment:
Both new readiness tests pass a hand-written `Supplier<String>` straight into `waitForReadiness`, so they cover the generic loop but not `clusterReadinessBlocker()` / `unmetSafeModeRules()` — which is where the compile break landed.
Those aren't untested outright: `TestLocalOzoneClusterRuntime` in `hadoop-ozone/integration-test` (untouched here) calls `cluster.start()` three times against a real SCM/OM/datanode, so it executes both the blocker and `stopServices()`'s `IOUtils.close(LOG, ...)`. Nothing asserts on the strings or the logging though, and neither file is in this PR's changed set, so from the diff alone both look uncovered.
If the logging is the point of the change, making the closeable list injectable would let a unit test hand in a throwing `AutoCloseable` and assert via `LogCapturer`.
Separately: `Duration.ofMillis(1)` is a bit misleading — the loop checks the blocker, then the deadline, then sleeps `READINESS_POLL_INTERVAL_MILLIS`, so iteration 1 never fires and this always costs a full 500 ms. Not flaky, just slower than it reads. Passing the poll interval as a parameter would fix it, since the tests already reach the method directly.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894208986
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
Review Comment:
I’m keeping the existing Hadoop configuration behavior here, where a unitless value is interpreted as milliseconds. --startup-timeout is a new CLI option, so requiring a unit there helps prevent accidental very short timeouts.
I added a test to make this distinction explicit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894235448
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -693,6 +883,7 @@ private void prepareStorageLayout() throws IOException {
switch (config.getFormatMode()) {
case ALWAYS:
+ LOG.info("Removing local Ozone data dir {} (format mode ALWAYS).", dataDir);
Review Comment:
I kept it as a log message because LocalOzoneCluster has no CLI output channel, and adding one for a single message feels outside this PR's scope.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894168847
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ /**
+ * Applies a replication factor the local runtime requires, reading the configured value the way
+ * {@link org.apache.hadoop.hdds.client.ReplicationConfig#parse} does, which accepts both the
+ * numeric and the named spelling.
+ *
+ * @throws IOException if the user configured {@code key} with a different factor
+ */
+ private void setLocalOverrideReplication(OzoneConfiguration conf, String key,
+ ReplicationFactor value) throws IOException {
+ String configured = conf.get(key);
+ if (configured != null && parseReplicationFactor(configured) != value) {
+ rejectUserConfigured(conf, key, value.name());
+ }
+ conf.set(key, value.name());
+ }
+
+ /**
+ * Throws when the value {@code conf} carries for {@code key} is the user's choice rather than a
+ * shipped default. The message names the source because the user has to find the value to
+ * remove it.
+ */
+ private static void rejectUserConfigured(OzoneConfiguration conf, String key, String required)
+ throws IOException {
+ String source = userConfiguredSource(conf, key);
+ if (source != null) {
+ throw new IOException("ozone local requires " + key + "=" + required
Review Comment:
Attempted to address the issue.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3769681163
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/OzoneLocal.java:
##########
@@ -197,13 +199,58 @@ static class RunCommand extends AbstractSubcommand implements Callable<Void> {
public Void call() throws Exception {
LocalOzoneClusterConfig config = resolveConfig();
try (LocalOzoneRuntime runtime = createRuntime(config, getOzoneConf())) {
- runtime.start();
+ start(runtime);
printSummary(runtime, config);
awaitShutdown(runtime);
}
return null;
}
+ /**
+ * Starts {@code runtime}, restating a failure in a form the user can act on. Service logs are
+ * off by default for this command, so the detail goes to the log for {@code --loglevel INFO}
+ * while the message keeps {@link GenericCli}'s single-line path: an exception with no message
+ * would otherwise print a raw stack trace.
+ */
+ private void start(LocalOzoneRuntime runtime) throws Exception {
+ try {
+ runtime.start();
+ } catch (Exception ex) {
+ LOG.error("Local Ozone cluster failed to start.", ex);
+ throw new IOException("Local Ozone failed to start: " + failureMessage(ex)
+ + " Re-run with `ozone --loglevel INFO local run` for service logs,"
+ + " or add --verbose for the full stack trace.", ex);
Review Comment:
I see your point. Since the user can use `--verbose` to show the error anyways, let's remove it :)
#### With `failureMessage(ex)` removed
```text
Local Ozone failed to start. Re-run with `ozone --loglevel INFO local run` for service logs, or add --verbose for the full stack trace.
```
Re-run with `--verbose`:
```text
java.io.IOException: Local Ozone failed to start. Re-run with `ozone --loglevel INFO local run` ...
at org.apache.hadoop.ozone.local.OzoneLocal$RunCommand.start(OzoneLocal.java:219)
at org.apache.hadoop.ozone.local.OzoneLocal$RunCommand.call(OzoneLocal.java:201)
... 71 more frames ...
Caused by: java.io.IOException: ozone local requires ozone.replication=ONE, but the configuration sets THREE (source: programmatically). Remove that value, or run with a configuration directory (OZONE_CONF_DIR) that does not set it.
```
#### With `failureMessage(ex)`
```text
java.io.IOException: Local Ozone failed to start: ozone local requires ozone.replication=ONE, but the configuration sets THREE (source: programmatically). Remove that value, or run with a configuration directory (OZONE_CONF_DIR) that does not set it. Re-run with `ozone --loglevel INFO local run` for service logs, or add --verbose for the full stack trace.
```
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894238147
##########
hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/local/TestLocalOzoneCluster.java:
##########
@@ -349,6 +354,226 @@ void prepareConfigurationRejectsTooManyDatanodes() throws Exception {
+ "; each datanode reserves 8 local ports.", error.getMessage());
}
+ @Test
+ void zeroDatanodesIsRejected() throws Exception {
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone"))
+ .setDatanodes(0)
+ .build();
+
+ IOException error = assertPrepareFails(config);
+
+ // configureLocalDefaults() requires one datanode for safe mode, so a zero-datanode cluster
+ // can never leave it; without this the run only fails when the readiness wait times out.
+ assertTrue(error.getMessage().contains("Datanode count 0"), error.getMessage());
+ }
+
+ @Test
+ void keyUnsetAfterBeingConfiguredIsNotRejected() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // Configuration#unset() drops the value but keeps the entry in updatingResource, so the key
+ // still reports a source with no value behind it.
+ seed.set(OZONE_METADATA_DIRS, "/somewhere/else", "test-ozone-site.xml");
+ seed.unset(OZONE_METADATA_DIRS);
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertNotNull(cluster.prepareConfiguration().getConfiguration().get(OZONE_METADATA_DIRS));
+ }
+ }
+
+ @Test
+ void defaultsFileNamedByPathCountsAsUserConfig() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // Only the shipped classpath resource is a default. A file the user pointed at with --conf
+ // is their choice however it is named.
+ seed.set(OZONE_REPLICATION, ReplicationFactor.THREE.name(), "/home/me/ozone-default.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ assertTrue(error.getMessage().contains(OZONE_REPLICATION), error.getMessage());
+ }
+
+ @Test
+ void tooManyDatanodesIsRejectedBeforeFormatDeletesDataDir() throws Exception {
+ Path dataDir = tempDir.resolve("local-ozone");
+ Path marker = writeMarker(dataDir, "keep me");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(dataDir)
+ .setFormatMode(LocalOzoneClusterConfig.FormatMode.ALWAYS)
+ .setDatanodes(LocalOzoneCluster.MAX_DATANODES + 1)
+ .build();
+
+ assertPrepareFails(config);
+
+ assertTrue(Files.exists(marker),
+ "format ALWAYS must not delete the data dir for a run that cannot start");
+ }
+
+ @Test
+ void conflictingUserConfigIsRejected() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(OZONE_REPLICATION, ReplicationFactor.THREE.name(), "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ // The user has to locate the value to remove it, so the message carries the key, the value
+ // ozone local requires, the configured value, and the source Configuration recorded.
+ String message = error.getMessage();
+ assertTrue(message.contains(OZONE_REPLICATION), message);
+ assertTrue(message.contains(ReplicationFactor.ONE.name()), message);
+ assertTrue(message.contains(ReplicationFactor.THREE.name()), message);
+ assertTrue(message.contains("test-ozone-site.xml"), message);
+ }
+
+ @Test
+ void userConfigMatchingTheLocalRequirementIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(OZONE_REPLICATION, ReplicationFactor.ONE.name(), "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertEquals(ReplicationFactor.ONE.name(),
+ cluster.prepareConfiguration().getConfiguration().get(OZONE_REPLICATION));
+ }
+ }
+
+ @Test
+ void conflictingUserConfigIsRejectedBeforeFormatDeletesDataDir() throws Exception {
+ Path dataDir = tempDir.resolve("local-ozone");
+ Path marker = writeMarker(dataDir, "keep me");
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(OZONE_REPLICATION, ReplicationFactor.THREE.name(), "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(dataDir)
+ .setFormatMode(LocalOzoneClusterConfig.FormatMode.ALWAYS)
+ .build();
+
+ assertPrepareFails(config, seed);
+
+ assertTrue(Files.exists(marker),
+ "format ALWAYS must not delete the data dir for a run that cannot start");
+ }
+
+ @Test
+ void equivalentDurationSpellingIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // Same interval the local runtime requires, written in another unit. Rejecting it would refuse
+ // to start over a value that means exactly what the runtime asked for.
+ seed.set(HDDS_HEARTBEAT_INTERVAL, "1000ms", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertEquals("1s", cluster.prepareConfiguration().getConfiguration()
+ .get(HDDS_HEARTBEAT_INTERVAL));
+ }
+ }
+
+ @Test
+ void equivalentBooleanSpellingIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(HDDS_CONTAINER_RATIS_ENABLED_KEY, "FALSE", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertFalse(cluster.prepareConfiguration().getConfiguration()
+ .getBoolean(HDDS_CONTAINER_RATIS_ENABLED_KEY, true));
+ }
+ }
+
+ @Test
+ void numericReplicationIsAccepted() throws Exception {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ // ReplicationConfig.parse() reads both "1" and "ONE", and compose environments in this repo
+ // write the numeric form, so it has to be accepted as the value the runtime requires.
+ seed.set(OZONE_SERVER_DEFAULT_REPLICATION_KEY, "1", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = new LocalOzoneCluster(config, seed)) {
+ assertEquals(ReplicationFactor.ONE.name(), cluster.prepareConfiguration()
+ .getConfiguration().get(OZONE_SERVER_DEFAULT_REPLICATION_KEY));
+ }
+ }
+
+ @Test
+ void conflictingDurationIsRejected() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(HDDS_HEARTBEAT_INTERVAL, "30s", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ // Comparing durations by value must not swallow a genuine conflict.
+ String message = error.getMessage();
+ assertTrue(message.contains(HDDS_HEARTBEAT_INTERVAL), message);
+ assertTrue(message.contains("30s"), message);
+ }
+
+ @Test
+ void unparseableValueIsRejected() {
+ OzoneConfiguration seed = new OzoneConfiguration();
+ seed.set(HDDS_HEARTBEAT_INTERVAL, "banana", "test-ozone-site.xml");
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ IOException error = assertPrepareFails(config, seed);
+
+ // A value the accessor cannot parse is a conflict, reported by the same message rather than
+ // escaping as a NumberFormatException from the comparison itself.
+ String message = error.getMessage();
+ assertTrue(message.contains(HDDS_HEARTBEAT_INTERVAL), message);
+ assertTrue(message.contains("banana"), message);
+ }
+
+ /**
+ * Regression guard: ozone-default.xml ships a value for most keys the local runtime requires
+ * (hdds.heartbeat.interval=30s, and so on) and is always on the classpath, so counting a shipped
+ * default as a user choice would reject every run.
+ */
+ @Test
+ void shippedDefaultIsNotTreatedAsUserConfig() throws Exception {
+ LocalOzoneClusterConfig config = LocalOzoneClusterConfig.builder(
+ tempDir.resolve("local-ozone")).build();
+
+ try (LocalOzoneCluster cluster = newCluster(config)) {
+ OzoneConfiguration prepared = cluster.prepareConfiguration().getConfiguration();
+
+ assertEquals("1s", prepared.get(HDDS_HEARTBEAT_INTERVAL));
+ assertNotEquals("1s", new OzoneConfiguration().get(HDDS_HEARTBEAT_INTERVAL),
+ "the shipped default must differ from the local value, or this guards nothing");
+ }
+ }
+
+ @Test
+ void readinessTimeoutNamesTheUnmetCondition() {
+ TimeoutException error = assertThrows(TimeoutException.class,
+ () -> LocalOzoneCluster.waitForReadiness(() -> "only 1 of 3 datanodes have registered",
Review Comment:
I added focused tests for the safe-mode rule formatting and changed the timeout test to use Duration.ZERO, so it no longer waits 500 ms.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on PR #10934:
URL: https://github.com/apache/ozone/pull/10934#issuecomment-5687081218
Merged. Thanks @henrybear327 for working on this, @peterxcli for review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3725442977
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/OzoneLocal.java:
##########
@@ -197,13 +199,58 @@ static class RunCommand extends AbstractSubcommand implements Callable<Void> {
public Void call() throws Exception {
LocalOzoneClusterConfig config = resolveConfig();
try (LocalOzoneRuntime runtime = createRuntime(config, getOzoneConf())) {
- runtime.start();
+ start(runtime);
printSummary(runtime, config);
awaitShutdown(runtime);
}
return null;
}
+ /**
+ * Starts {@code runtime}, restating a failure in a form the user can act on. Service logs are
+ * off by default for this command, so the detail goes to the log for {@code --loglevel INFO}
+ * while the message keeps {@link GenericCli}'s single-line path: an exception with no message
+ * would otherwise print a raw stack trace.
+ */
+ private void start(LocalOzoneRuntime runtime) throws Exception {
+ try {
+ runtime.start();
+ } catch (Exception ex) {
+ LOG.error("Local Ozone cluster failed to start.", ex);
+ throw new IOException("Local Ozone failed to start: " + failureMessage(ex)
+ + " Re-run with `ozone --loglevel INFO local run` for service logs,"
+ + " or add --verbose for the full stack trace.", ex);
Review Comment:
I don't think we need the failureMessage extraction here. Since we are already passing ex as the cause in new `IOException("...", ex)`.
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -196,6 +201,7 @@ public final class LocalOzoneCluster implements LocalOzoneRuntime {
private StorageContainerManager scm;
private OzoneManager om;
private final List<HddsDatanodeService> datanodes = new ArrayList<>();
+ private final List<String> discardedUserConfigKeys = new ArrayList<>();
Review Comment:
I am not entirely sure about the purpose of discardedUserConfigKeys. In my opinion, if a user specifies a parameter and it's invalid/incompatible, we should fail-fast and throw an error message, rather than silently modifying it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on PR #10934:
URL: https://github.com/apache/ozone/pull/10934#issuecomment-5280107642
PTAL @peterxcli , thank you!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3897560556
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
Review Comment:
After some thinking, I decided to reject time configuration input without unit, so we can eliminate the problematic time handling parsing part of the logic. WDYT?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894178198
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ /**
+ * Applies a replication factor the local runtime requires, reading the configured value the way
+ * {@link org.apache.hadoop.hdds.client.ReplicationConfig#parse} does, which accepts both the
+ * numeric and the named spelling.
+ *
+ * @throws IOException if the user configured {@code key} with a different factor
+ */
+ private void setLocalOverrideReplication(OzoneConfiguration conf, String key,
+ ReplicationFactor value) throws IOException {
+ String configured = conf.get(key);
+ if (configured != null && parseReplicationFactor(configured) != value) {
+ rejectUserConfigured(conf, key, value.name());
+ }
+ conf.set(key, value.name());
+ }
+
+ /**
+ * Throws when the value {@code conf} carries for {@code key} is the user's choice rather than a
+ * shipped default. The message names the source because the user has to find the value to
+ * remove it.
+ */
+ private static void rejectUserConfigured(OzoneConfiguration conf, String key, String required)
+ throws IOException {
+ String source = userConfiguredSource(conf, key);
+ if (source != null) {
+ throw new IOException("ozone local requires " + key + "=" + required
+ + ", but the configuration sets " + conf.get(key) + " (source: " + source
+ + "). Remove that value, or run with a configuration directory (OZONE_CONF_DIR)"
+ + " that does not set it.");
+ }
+ }
+
+ private static boolean matchesInt(OzoneConfiguration conf, String key, int value) {
+ try {
+ return conf.getInt(key, value) == value;
+ } catch (NumberFormatException unreadable) {
+ // A value the accessor cannot read is a conflict; the caller reports it by key.
+ return false;
+ }
+ }
+
+ private static boolean matchesDuration(OzoneConfiguration conf, String key,
+ long requiredMillis) {
+ try {
+ return conf.getTimeDuration(key, requiredMillis, TimeUnit.MILLISECONDS) == requiredMillis;
+ } catch (NumberFormatException unreadable) {
+ return false;
+ }
+ }
+
+ /** Returns the factor {@code value} names in either spelling, or null if it names neither. */
+ private static ReplicationFactor parseReplicationFactor(String value) {
+ String trimmed = value.trim();
+ try {
+ return ReplicationFactor.valueOf(Integer.parseInt(trimmed));
+ } catch (IllegalArgumentException notNumeric) {
+ try {
+ return ReplicationFactor.valueOf(trimmed);
+ } catch (IllegalArgumentException notNamed) {
+ return null;
+ }
+ }
+ }
+
+ /**
+ * Returns where {@code key} got the value the user chose, or null if the user chose none. A
+ * value whose last source is the shipped ozone-default.xml is a default, not a user choice.
+ * The comparison is exact: Configuration records a classpath resource by its bare name, so a
+ * file the user named with {@code --conf} keeps its path here and stays a user choice however
+ * that file is called.
+ */
+ private static String userConfiguredSource(OzoneConfiguration conf, String key) {
+ String[] sources = conf.getPropertySources(key);
+ if (sources == null || sources.length == 0) {
+ return null;
+ }
+ String source = sources[sources.length - 1];
+ return OZONE_DEFAULT_XML.equals(source) ? null : source;
Review Comment:
the implementation now derives all *-default.xml names from OzoneConfiguration.getConfigurationResourceFiles()
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894163201
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -351,28 +363,25 @@ private void stopScm() {
}
}
- private void configureLocalDefaults(OzoneConfiguration conf) {
- conf.set(OZONE_METADATA_DIRS, metadataDir().toString());
+ private void configureLocalDefaults(OzoneConfiguration conf) throws IOException {
+ setLocalOverride(conf, OZONE_METADATA_DIRS, metadataDir().toString());
Review Comment:
> Suggestion: keep plain conf.set() for keys whose required value is a path the runtime generates and owns, and reserve setLocalOverride for keys where a conflicting user value would actually change observable cluster behaviour.
Attempted to follow this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "chungen0126 (via GitHub)" <gi...@apache.org>.
chungen0126 merged PR #10934:
URL: https://github.com/apache/ozone/pull/10934
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on PR #10934:
URL: https://github.com/apache/ozone/pull/10934#issuecomment-5188058123
@chungen0126 PTAL if this is what you meant in https://github.com/apache/ozone/pull/10757#pullrequestreview-4734315001, thanks.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3769570612
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -196,6 +201,7 @@ public final class LocalOzoneCluster implements LocalOzoneRuntime {
private StorageContainerManager scm;
private OzoneManager om;
private final List<HddsDatanodeService> datanodes = new ArrayList<>();
+ private final List<String> discardedUserConfigKeys = new ArrayList<>();
Review Comment:
Indeed I went to far in designing the error handling :(
`setLocalOverride` now throws.
The whole `discardedUserConfigKeys` code path is removed.
PTAL, thank you!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894160320
##########
hadoop-ozone/tools/pom.xml:
##########
@@ -46,6 +46,10 @@
<groupId>jakarta.xml.bind</groupId>
<artifactId>jakarta.xml.bind-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
Review Comment:
commons-lang3 was removed and hdds-interface-admin declared in hadoop-ozone/tools/pom.xml:70.
Maven dependency analysis now passes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894188233
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -643,30 +780,64 @@ private void startDatanodes(List<OzoneConfiguration> datanodeConfigurations) {
}
private void waitForClusterReadiness(Duration timeout) throws Exception {
+ waitForReadiness(this::clusterReadinessBlocker, "Ozone cluster", timeout);
+ }
+
+ /**
+ * Polls {@code blocker} until it reports ready. {@code blocker} returns why {@code subject} is
+ * not ready yet, so the wait can name the unmet condition instead of reporting a bare timeout
+ * that cannot distinguish a slow start from a stuck one.
+ */
+ static void waitForReadiness(Supplier<String> blocker, String subject, Duration timeout)
+ throws InterruptedException, TimeoutException {
long deadlineNanos = System.nanoTime() + timeout.toNanos();
+ long nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
while (true) {
- if (isClusterReady()) {
+ String reason = blocker.get();
+ if (reason == null) {
return;
}
if (System.nanoTime() >= deadlineNanos) {
- throw new TimeoutException("Timed out waiting " + timeout
- + " for the local Ozone cluster to become ready.");
+ throw new TimeoutException("Timed out waiting " + timeout + " for the local " + subject
+ + " to become ready: " + reason + ".");
+ }
+ if (System.nanoTime() >= nextLogNanos) {
+ LOG.info("Waiting for the local {} to become ready: {}.", subject, reason);
+ nextLogNanos = System.nanoTime() + READINESS_LOG_INTERVAL_NANOS;
}
Thread.sleep(READINESS_POLL_INTERVAL_MILLIS);
}
}
- private boolean isClusterReady() {
- if (!scm.checkLeader() || !om.isLeaderReady()) {
- return false;
+ /**
+ * Returns why the cluster is not usable yet, or null once it is ready. The cluster is usable
+ * once SCM and OM are leader-ready, every datanode has registered with SCM, and SCM has left
+ * safe mode.
+ */
+ private String clusterReadinessBlocker() {
+ if (!scm.checkLeader()) {
+ return "SCM has no Ratis leader yet";
+ }
+ if (!om.isLeaderReady()) {
+ return "OM is not leader-ready yet";
}
- if (config.getDatanodes() == 0) {
- return true;
+ int registered = scm.getScmNodeManager().getAllNodes().size();
+ if (registered < config.getDatanodes()) {
+ return "only " + registered + " of " + config.getDatanodes()
+ + " datanodes have registered with SCM";
}
- // The cluster is usable once every datanode has registered with SCM and
- // SCM has left safe mode.
- return scm.getScmNodeManager().getAllNodes().size() >= config.getDatanodes()
- && !scm.isInSafeMode();
+ // Registration alone is not enough: SCM refuses block allocation until it leaves safe mode.
+ if (scm.isInSafeMode()) {
Review Comment:
Addressed
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15986. Report ozone local startup failures [ozone]
Posted by "henrybear327 (via GitHub)" <gi...@apache.org>.
henrybear327 commented on code in PR #10934:
URL: https://github.com/apache/ozone/pull/10934#discussion_r3894208986
##########
hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/local/LocalOzoneCluster.java:
##########
@@ -381,6 +390,141 @@ private void configureLocalDefaults(OzoneConfiguration conf) {
conf.setFromObject(scmClientConfig);
}
+ /**
+ * Applies a value the local runtime requires, rejecting a conflicting one the user configured.
+ * These keys are set rather than {@code setIfUnset} because ozone-default.xml would otherwise
+ * win; a user value is refused rather than replaced, so the cluster never behaves differently
+ * from the configuration the user is reading. Rejecting here, at the point of the override,
+ * keeps a later override from being added without the same check.
+ *
+ * <p>This overload compares text, for keys whose value carries no other spelling. The typed
+ * overloads below compare through the accessor the services read the key with, so a value that
+ * already means what the runtime requires is kept rather than rejected.</p>
+ *
+ * @throws IOException if the user configured {@code key} with a value other than {@code value}
+ */
+ private void setLocalOverride(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ // Configuration#unset() leaves the key in updatingResource, so a source can outlive its
+ // value; there is nothing to reject when no value is configured.
+ if (conf.get(key) != null && !value.equals(conf.get(key))) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, boolean value)
+ throws IOException {
+ // Defaulting to the negation keeps a value getBoolean() cannot read from matching by accident.
+ if (conf.get(key) != null && conf.getBoolean(key, !value) != value) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setBoolean(key, value);
+ }
+
+ private void setLocalOverride(OzoneConfiguration conf, String key, int value)
+ throws IOException {
+ if (conf.get(key) != null && !matchesInt(conf, key, value)) {
+ rejectUserConfigured(conf, key, String.valueOf(value));
+ }
+ conf.setInt(key, value);
+ }
+
+ /**
+ * Applies a duration the local runtime requires. The configured value is compared as a duration
+ * rather than as text, so the same length written in another unit is not treated as a conflict.
+ *
+ * @throws IOException if the user configured {@code key} with a different duration
+ */
+ private void setLocalOverrideDuration(OzoneConfiguration conf, String key, String value)
+ throws IOException {
+ long requiredMillis = TimeDurationUtil.getTimeDurationHelper(key, value, TimeUnit.MILLISECONDS);
+ if (conf.get(key) != null && !matchesDuration(conf, key, requiredMillis)) {
+ rejectUserConfigured(conf, key, value);
+ }
+ conf.set(key, value);
+ }
+
Review Comment:
I’m keeping the existing Hadoop configuration behavior here, where a unitless value is interpreted as milliseconds. --startup-timeout is a new CLI option, so requiring a unit there helps prevent accidental very short timeouts.
I added a test to make this distinction explicit.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org