Copilot commented on code in PR #9282:
URL: https://github.com/apache/ozone/pull/9282#discussion_r3472128228
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HAUtils.java:
##########
@@ -97,10 +98,21 @@ public static ScmInfo getScmInfo(OzoneConfiguration conf)
throws IOException {
configuration.setFromObject(scmClientConfig);
}
return getScmBlockClient(configuration).getScmInfo();
+ } catch (ConfigurationException e) {
+ LOG.error("Failed to get SCM info", e);
+ throw new IOException("Failed to get SCM info due to configuration
error.\n" +
+ "Please verify SCM HA configuration properties:\n" +
+ " - ozone.scm.service.ids\n" +
Review Comment:
`getScmInfo` now logs `LOG.error(...)` in each catch and then rethrows,
which will generally be logged again by callers (eg
`StorageContainerManagerStarter.bootStrapScm`) and by the CLI exception
handler. This can lead to duplicated/triplicated error logs for a single config
typo. Consider letting the exception propagate and logging only at the
top-level entrypoint.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/scm/proxy/SCMFailoverProxyProviderBase.java:
##########
@@ -155,13 +155,22 @@ protected synchronized void loadConfigs() {
throw new ConfigurationException(protocolClass.getSimpleName() + " SCM
Address could not " +
"be obtained from config. Config is not properly defined");
} else {
- InetSocketAddress protocolAddr =
NetUtils.createSocketAddr(protocolAddress);
-
- String scmServiceId = scmNodeInfo.getServiceId();
- String scmNodeId = scmNodeInfo.getNodeId();
- scmNodeIds.add(scmNodeId);
- SCMProxyInfo scmProxyInfo = new SCMProxyInfo(scmServiceId, scmNodeId,
protocolAddr);
- scmProxyInfoMap.put(scmNodeId, scmProxyInfo);
+ try {
+ InetSocketAddress protocolAddr =
NetUtils.createSocketAddr(protocolAddress);
+
+ String scmServiceId = scmNodeInfo.getServiceId();
+ String scmNodeId = scmNodeInfo.getNodeId();
+ scmNodeIds.add(scmNodeId);
+ SCMProxyInfo scmProxyInfo = new SCMProxyInfo(scmServiceId,
scmNodeId, protocolAddr);
+ scmProxyInfoMap.put(scmNodeId, scmProxyInfo);
+ } catch (IllegalArgumentException e) {
+ throw new ConfigurationException(
+ "Invalid SCM address format: '" + protocolAddress + "'.\n" +
+ "Expected format: 'hostname:port'.\n" +
+ "This often happens when 'ozone.scm.service.ids' is misspelled
as 'ozone.scm.service.id'.\n" +
+ "Please check your SCM HA configuration.\n" +
+ "Original error: " + e.getMessage());
+ }
Review Comment:
The new `IllegalArgumentException` handling drops the original exception as
a cause, which makes it harder to debug the underlying parsing failure (and
loses stack trace/context). Wrap it as the cause when rethrowing
`ConfigurationException`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/StorageContainerManagerStarter.java:
##########
@@ -128,7 +128,13 @@ public void initScm(@CommandLine.Option(names = {
"--clusterid" },
public void bootStrapScm()
throws Exception {
commonInit();
- boolean result = receiver.bootStrap(conf);
+ boolean result;
+ try {
+ result = receiver.bootStrap(conf);
+ } catch (IOException e) {
+ LOG.error("SCM bootstrap failed", e);
+ throw e;
+ }
Review Comment:
Catching + logging + rethrowing here can create duplicated error output:
`GenericCli` already prints the exception message for CLI failures, and
`HAUtils.getScmInfo()` currently also logs on failure (if kept). If the goal is
to surface a clear operator-facing message, consider logging the exception
message (without full stack) at ERROR and logging the stack trace at DEBUG to
reduce noise while keeping details available.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMNodeInfo.java:
##########
@@ -77,6 +78,7 @@ public static List<SCMNodeInfo>
buildNodeInfo(ConfigurationSource conf) {
List<SCMNodeInfo> scmNodeInfoList = new ArrayList<>();
String scmServiceId = HddsUtils.getScmServiceId(conf);
if (scmServiceId != null) {
+ LOG.info("{} for StorageContainerManager is {}",
OZONE_SCM_SERVICE_IDS_KEY, scmServiceId);
ArrayList< String > scmNodeIds = new ArrayList<>(
HddsUtils.getSCMNodeIds(conf, scmServiceId));
Review Comment:
The PR description mentions a proactive check in
`SCMNodeInfo.buildNodeInfo()` to detect the common typo `ozone.scm.service.id`,
but the current change only adds INFO logs and still silently falls back to
non-HA config when the HA key is misspelled. Consider explicitly detecting the
misspelled key and throwing a `ConfigurationException` with a targeted message
(and optionally downgrade the new INFO log to DEBUG to avoid log noise).
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/HAUtils.java:
##########
@@ -97,10 +98,21 @@ public static ScmInfo getScmInfo(OzoneConfiguration conf)
throws IOException {
configuration.setFromObject(scmClientConfig);
}
return getScmBlockClient(configuration).getScmInfo();
+ } catch (ConfigurationException e) {
+ LOG.error("Failed to get SCM info", e);
+ throw new IOException("Failed to get SCM info due to configuration
error.\n" +
+ "Please verify SCM HA configuration properties:\n" +
Review Comment:
This change introduces a new user-facing error message for configuration
problems in `getScmInfo` (multi-line, with specific properties). There are
existing unit tests covering other `HAUtils` helpers (eg
`HAUtils.getExistingFiles(...)`), but no automated coverage validating this new
message/exception wrapping behavior. Adding a focused unit test would help
prevent regressions in operator-facing diagnostics.
--
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]
