Skip to content

HDDS-15997. Fix QuotaRepairTask scan hang and partial counts on worker failure - #10884

Open
chihsuan wants to merge 3 commits into
apache:masterfrom
chihsuan:HDDS-15997
Open

HDDS-15997. Fix QuotaRepairTask scan hang and partial counts on worker failure#10884
chihsuan wants to merge 3 commits into
apache:masterfrom
chihsuan:HDDS-15997

Conversation

@chihsuan

@chihsuan chihsuan commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

QuotaRepairTask scans each table with one producer thread feeding batches to worker threads through a bounded queue. If all workers die with an exception, the producer blocks forever on the full queue and the repair hangs with IN_PROGRESS held. If the scan is interrupted, both sides return silently and the partially scanned counts are submitted through Ratis as a successful repair.

This change makes scan failures fail the repair instead:

  1. Extract the scan loop into scanTableInBatches with an injectable per-record consumer.
  2. Enqueue batches via putBatch, which fails fast when a worker has already exited.
  3. Treat interruption as a scan failure on both producer and worker side.
  4. On failure, still await all workers and scan tasks before returning, since the DB checkpoint is deleted in a finally block and no worker may outlive it.

New unit tests cover worker failure, producer interrupt, and awaiting in-flight workers. This is a prerequisite for HDDS-15835, which adds the first per-record consumer that reads RocksDB and makes worker failure realistic.

What is the link to the Apache JIRA

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

How was this patch tested?

Focused unit tests passed (7 tests, no failures):

mvn -pl :ozone-manager test -Dtest=TestQuotaRepairTask -DskipShade -DskipRecon -DskipDocs

Repository checkstyle also passed:

./hadoop-ozone/dev-support/checks/checkstyle.sh

Generated-by: Codex (GPT-5)

Copilot AI review requested due to automatic review settings July 28, 2026 10:15

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chungen0126 chungen0126 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.

Thanks @chihsuan for working on this.

for (Future<?> f : tasks) {
if (f.isDone()) {
f.get();
throw new IllegalStateException("quota repair scan worker exited prematurely");

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.

Could you help clarify when this scenario occurs?

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.

Good question. It's a safety guard, not a case we expect to hit. In theory, it should be unreachable today, since a worker can't finish normally while the producer is still feeding it, so a missing worker always comes with an exception we fail on.

The reason it's still there is that this PR changes the producer to poll instead of blocking forever, so the loop needs a guaranteed exit. If workers ever went away without an exception, nobody would drain the queue, and the producer would just keep polling. Does that make sense to you?

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.

@chungen0126 Do you have any feedback on this? Or do you prefer to remove this?

@chungen0126 chungen0126 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.

Overall, LGTM. Just one minor.

kvList = new ArrayList<>(BATCH_SIZE);
}
}
q.put(kvList);
putBatch(q, kvList, tasks);

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.

If the total number of keys is exactly a multiple of the batch size, this putBatch will put an empty kvList. I think it would be better to add a condition here to check if kvList is not empty before putting it.

@chihsuan chihsuan Aug 19, 2026

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.

Fixed, the trailing batch is now only queued when non-empty. Thanks!

@chihsuan
chihsuan requested a review from chungen0126 August 19, 2026 12:50

@chungen0126 chungen0126 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 LGTM

@chungen0126
chungen0126 requested a review from jojochuang August 20, 2026 16:40
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