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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
