Skip to content

HDDS-15599. Follow up clean up from HDDS-15531 PR 10486#10538

Merged
adoroszlai merged 1 commit into
masterfrom
HDDS-15599-followup-cleanup
Jun 18, 2026
Merged

HDDS-15599. Follow up clean up from HDDS-15531 PR 10486#10538
adoroszlai merged 1 commit into
masterfrom
HDDS-15599-followup-cleanup

Conversation

@kerneltime

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Post-merge cleanup addressing @szetszwo's review comments on #10486
(HDDS-15531). He left three minor comments after the merge and asked that
they be handled in a follow-up PR. All three are behavior-preserving.

  1. ConnectionFailureUtilsConnectException and
    NoRouteToHostException both extend SocketException, so their explicit
    instanceof checks were redundant. Removed them (the SocketException
    branch still matches both) and added a comment. They remain listed in the
    class Javadoc as connection-failure shapes.
    (comment)

  2. OzoneConfigKeys — moved ozone.client.failover.resolve-needed
    (key, default, and Javadoc) next to ozone.client.failover.max.attempts.
    (comment)

  3. OMFailoverProxyProviderBase.maybeRefreshCurrentOmAddress — replaced
    the synchronized null-guard with Objects.requireNonNull for nodeId
    and info. getCurrentProxyOMNodeId() is already synchronized and
    omProxies is an unmodifiable map built once at construction, so neither
    read needs the provider monitor and both values are always present.
    (comment)

No functional changes: the connection-failure classification is identical
(the removed types still match via SocketException), the config constant is
unchanged, and requireNonNull only fast-fails on inputs that cannot occur
rather than silently returning false.

What is the link to the Apache JIRA?

https://issues.apache.org/jira/browse/HDDS-15599

How was this patch tested?

No new tests — these are refactors covered by the existing suites:

  • TestConnectionFailureUtils: ConnectException / NoRouteToHostException
    (bare and cause-nested) remain classified as connection failures via the
    SocketException branch.
  • TestOMFailoverProxyProviderRefreshWired and TestOMProxyInfoDnsRefresh:
    the refresh wiring is exercised through method overrides, so the internal
    refactor of maybeRefreshCurrentOmAddress does not change their behavior.

mvn checkstyle:check reports 0 violations on both hadoop-hdds/common and
hadoop-ozone/common.

Addresses the post-merge review comments from @szetszwo on #10486. All
three are minor, behavior-preserving changes he asked to be handled in a
follow-up PR.

- ConnectionFailureUtils: ConnectException and NoRouteToHostException both
  extend SocketException, so their explicit instanceof checks were
  redundant. Removed them (the SocketException branch still matches both)
  and added a comment. They remain listed in the class Javadoc as
  connection-failure shapes.

- OzoneConfigKeys: moved ozone.client.failover.resolve-needed (key,
  default, and Javadoc) next to ozone.client.failover.max.attempts.

- OMFailoverProxyProviderBase: replaced the synchronized null-guard in
  maybeRefreshCurrentOmAddress with Objects.requireNonNull for nodeId and
  info. getCurrentProxyOMNodeId() is already synchronized and omProxies is
  an unmodifiable map built once at construction, so neither read needs the
  provider monitor and both values are always present.

@szetszwo szetszwo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 the change looks good.

@adoroszlai adoroszlai merged commit d3135c7 into master Jun 18, 2026
94 checks passed
@adoroszlai

Copy link
Copy Markdown
Contributor

Thanks @kerneltime for the patch, @szetszwo for the review.

@adoroszlai adoroszlai deleted the HDDS-15599-followup-cleanup branch June 18, 2026 07:20
@adoroszlai

Copy link
Copy Markdown
Contributor

@kerneltime please push dev branches (e.g. HDDS-15599-followup-cleanup) to your fork, not apache/ozone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants