Skip to content

HDDS-17724. Limit command-status report size in datanode heartbeats. - #11962

Draft
sadanand48 wants to merge 9 commits into
apache:masterfrom
sadanand48:HDDS-17724
Draft

sadanand48 wants to merge 9 commits into
apache:masterfrom
sadanand48:HDDS-17724

Conversation

@sadanand48

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

HDDS-16723 added a count-based limit for reports in datanode heartbeats, but it did not limit their serialized size. A small number of command-status reports containing accumulated delete-block ACKs could still exceed ipc.maximum.data.length.

This PR:

  • Adds hdds.heartbeat.max.command.status.report.size, defaulting to 64MB.
  • Limits command-status data to the smaller of: the configured limit; or the remaining space within 90% of ipc.maximum.data.length.
  • Adds command-status reports after other heartbeat fields so their available budget reflects existing payload size.
  • Splits aggregate reports only between complete CommandStatus entries.
  • Never splits an individual delete ACK or its result list.
  • Leaves deferred reports queued for subsequent scheduled heartbeats.
  • Removes immediate follow-up heartbeats to avoid burst RPCs during backlog recovery.
  • Preserves selected reports and ordering when an RPC fails.
  • Adds per-endpoint metrics for queued, sent, deferred, requeued, and oversized command-status bytes.
  • Adds tests covering byte budgeting, splitting, ordering, retry, and heartbeat scheduling.
  • The existing report-count limit remains in place for full reports and ICR backlogs. This change adds a complementary byte-based safeguard for command-status reports.

What is the link to the Apache JIRA

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

How was this patch tested?

Unit tests

Developed with help of Cursor (GPT-5.6 Sol)

…/java/org/apache/hadoop/ozone/container/common/states/endpoint/TestHeartbeatEndpointTask.java

@smengcl smengcl 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 @sadanand48 for the patch

if (context.hasPendingReports(rpcEndpoint.getAddress())) {
// addReports was capped at maxReportsPerHB and left reports queued. Trigger immediately.
context.getParent().setNextHB(Time.monotonicNow());
if (context.getQueueMetrics() != null) {

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.

Removing the immediate follow-up heartbeat changes behavior for all queued reports, not only command statuses. An ICR backlog, or a full report left behind by the count cap, now drains one hdds.heartbeat.report.max.limit batch per heartbeat interval instead of right away, which is what HDDS-16723 added the follow-up for. Could we keep it here, or move this part to its own JIRA?

If it stays, the hdds.heartbeat.report.max.limit description in ozone-default.xml still says reports beyond the limit "are sent in an immediate follow-up heartbeat", so it needs an update. Also hasPendingReports() and getAllAvailableReports(endpoint, maxLimit) would only be used by tests after this, so they could go instead of updating the javadoc.

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, this behavior change is intentional. The byte limit bounds each heartbeat payload, but immediate follow-ups could still send repeated 64 MB heartbeats in a tight loop while draining a large backlog, potentially recreating RPC and heap pressure on a recovering SCM. Waiting for the normal heartbeat interval bounds both payload size and delivery rate.

The default count limit is 10,000 top-level reports per datanode per heartbeat, while an FCR counts as only one report, so the expected backlog recovery rate should remain reasonable.

I would prefer to keep this change in the same JIRA because pacing backlog recovery is part of the mitigation. I will update the stale ozone-default.xml description

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.

I can see the problem, CommandStatusReportPublisher is combining all available command in last 60sec. Normally it can have 2-3 last command set only.

but incrementalReportsQueue can have CommandStatusReportsProto as list of large number which actually can cause issue. Single CommandStatusReportsProto will not be that big.

But on safer size, I think we can limit the size to 2MB chunk in CommandStatusReportPublisher is better implementation, then splitting at later stage. there we can pick 2MB max size list to build response to fill the gap.

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.

A 2 MB publisher chunk can only be a target, not a guaranteed limit, because one CommandStatus may itself exceed 2 MB and its ACK results cannot be split. That larger status would still require the oversized progress path.

Also, accumulated reports are not sent one per heartbeat. Heartbeat selection packs multiple queued reports until the total command-status byte or report-count budget is reached, with the remainder sent in subsequent scheduled heartbeats. Therefore, publisher chunking does not replace endpoint-level total budgeting or oversized-status handling.

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.

As per default configuration for max number of transactions from SCM, 500k * 60Bytes approx = 3-4MB max.
So each transaction will not go to huge number and keeping each response one-by-one will be helpful and split is not required.

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.

500k × 60 bytes is approximately 30 MB , not 3–4 MB. Also, the 500k limit is configurable and is a transaction/block-count limit rather than a serialized-byte guarantee.

More importantly, CommandStatusReportPublisher currently combines all available CommandStatus entries into one CommandStatusReportsProto. Therefore, several individually bounded responses can still produce an aggregate exceeding the 64 MB or safe RPC budget.

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.

CommandStatusReportPublisher -- needs to change and avoid combine the status. So this will make each to be 30MB by default and no need to split.

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.

CommandStatusReportPublisher currently returns one CommandStatusReportsProto per execution, and the generic ReportPublisher enqueues one message. Producing one report per CommandStatus in the same cycle therefore requires changing the generic publisher contract to publish a collection of reports.

If only getReport() is changed to return one command status, the remaining statuses would drain one per command-status report interval, which is 60 seconds by default. This moves the splitting upstream and broadens the change; it does not simply remove splitting. Are you proposing that we change ReportPublisher to support publishing multiple reports per execution?

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

@sadanand48 given few comment

@sadanand48
sadanand48 requested a review from smengcl November 9, 2026 16:10
@sadanand48
sadanand48 requested a review from smengcl November 12, 2026 11:25

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

lgtm.

@sumitagrawl Do you want to take another look?

@adoroszlai
adoroszlai marked this pull request as ready for review November 13, 2026 07:41
@adoroszlai

Copy link
Copy Markdown
Contributor

Thanks @sadanand48 for the patch.

Workflows are not currently being run on this fork due to the scale of GitHub Actions usage. A repository maintainer can choose to re-enable these

Can you please re-enable workflows in your fork? Afterwards you may want to manually disable some:

@sadanand48
sadanand48 marked this pull request as draft November 13, 2026 09:04
Comment on lines +79 to +80
public static final String HDDS_IPC_MAXIMUM_DATA_LENGTH =
"ipc.maximum.data.length";

@adoroszlai adoroszlai Nov 13, 2026 •

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.

@sadanand48 TestOzoneConfigurationFields failed. ipc.maximum.data.length should not be declared in HddsConfigKeys (since ozone-default.xml does not have it, nor should it), but in a non-config class (which is not checked by TestOzoneConfigurationFields). For now it can be moved to e.g. HeartbeatEndpointTask.

@adoroszlai

Copy link
Copy Markdown
Contributor

@sadanand48 Please don't re-run failed workflow of the outdated commit (853c5f8), it cancels the workflow for the newer commit (d35de55).

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants