Skip to content

HDDS-15287. Remove unused deferred/async RPC#10310

Merged
adoroszlai merged 5 commits into
apache:masterfrom
adoroszlai:HDDS-15287
May 19, 2026
Merged

HDDS-15287. Remove unused deferred/async RPC#10310
adoroszlai merged 5 commits into
apache:masterfrom
adoroszlai:HDDS-15287

Conversation

@adoroszlai
Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai commented May 19, 2026

What changes were proposed in this pull request?

Same as #10280, plus:

  • removed async call from forked hadoop.io.retry package (aad36a6)
  • merged master to resolve conflicts

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

How was this patch tested?

https://github.com/adoroszlai/ozone/actions/runs/26091212865

@adoroszlai adoroszlai self-assigned this May 19, 2026
@adoroszlai adoroszlai added the code-cleanup Changes that aim to make code better, without changing functionality. label May 19, 2026
@adoroszlai adoroszlai changed the title HDDS-15287. Remove unused deferred/async RPC (#10280) HDDS-15287. Remove unused deferred/async RPC May 19, 2026
@adoroszlai adoroszlai requested review from ivandika3 and szetszwo May 19, 2026 11:48
Copy link
Copy Markdown
Contributor

@ivandika3 ivandika3 left a comment

Choose a reason for hiding this comment

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

LGTM +1.

Comment on lines 124 to 125
* If the wait time is positive, it either sleeps for synchronous calls
* or immediately returns for asynchronous calls.
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.

Nit: Remove asynchronous calls, I'm OK to leave it be.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'll leave it for now, but while checking it, I spotted a potential bug (both here and in Hadoop):

  1. RetryInfo.retryTime is calculated when it is created:
    this.delay = delay;
    this.retryTime = Time.monotonicNow() + delay;
  2. waitTime is calculated before sleep as:
    synchronized Long getWaitTime(final long now) {
    return retryInfo == null? null: retryInfo.retryTime - now;
  3. but then original RetryInfo.delay is used for actual sleep time:
    final Long waitTime = getWaitTime(Time.monotonicNow());
    LOG.trace("#{} processRetryInfo: retryInfo={}, waitTime={}",
    callId, retryInfo, waitTime);
    if (waitTime != null && waitTime > 0) {
    try {
    Thread.sleep(retryInfo.delay);

What do you think?

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.

You are right that it is a bug -- it should use waitTime to sleep.

Copy link
Copy Markdown
Contributor

@szetszwo szetszwo left a comment

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 92374bc into apache:master May 19, 2026
49 of 50 checks passed
@adoroszlai
Copy link
Copy Markdown
Contributor Author

Thanks @ivandika3, @szetszwo for the repeated review.

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

Labels

code-cleanup Changes that aim to make code better, without changing functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants