smengcl commented on code in PR #11423:
URL: https://github.com/apache/ozone/pull/11423#discussion_r4268163779
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/StateContext.java:
##########
@@ -375,16 +375,33 @@ public void putBackReports(List<Message> reportsToPutBack,
*/
public List<Message> getAllAvailableReports(
HostAndPort endpoint) {
- int maxLimit = Integer.MAX_VALUE;
- // TODO: It is highly unlikely that we will reach maxLimit for the number
- // for the number of reports, specially as it does not apply to the
- // number of entries in a report. But if maxLimit is hit, should a
- // heartbeat be scheduled ASAP? Should full reports not included be
- // dropped? Currently this code will keep the full reports not sent
- // and include it in the next heartbeat.
+ return getAllAvailableReportsUpToLimit(endpoint, Integer.MAX_VALUE);
+ }
+
+ /**
+ * Returns up to {@code maxLimit} available reports for the endpoint,
+ * removing them from the queue. Any reports beyond the limit stay queued
+ * and are returned by a later call; callers can use {@link
+ * #hasPendingReports(HostAndPort)} to detect that case and schedule a
+ * follow-up heartbeat.
+ *
+ * @return List of reports
+ */
+ public List<Message> getAllAvailableReports(
+ HostAndPort endpoint, int maxLimit) {
return getAllAvailableReportsUpToLimit(endpoint, maxLimit);
}
+ /**
+ * Returns true if incremental reports are still queued for the endpoint.
+ */
+ public boolean hasPendingReports(HostAndPort endpoint) {
Review Comment:
Please include pending full reports here as well. With
`hdds.container.report.max.limit=1` and
`hdds.node.report.interval=hdds.heartbeat.interval` (an allowed configuration),
each regular heartbeat consumes the newly refreshed node report first. The
container and pipeline reports remain ready, but this method returns false, so
`HeartbeatEndpointTask` never schedules the follow-up needed to send them. On
an idle datanode without incremental reports, SCM can therefore stop receiving
those periodic snapshots indefinitely. Checking both the ready flag and the
report value also avoids scheduling repeatedly for a report that has not been
generated yet.
```diff
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/StateContext.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/StateContext.java
@@ -393,13 +393,18 @@
}
/**
- * Returns true if incremental reports are still queued for the endpoint.
+ * Returns true if any reports are still queued for the endpoint.
*/
public boolean hasPendingReports(HostAndPort endpoint) {
synchronized (incrementalReportsQueue) {
List<Message> reportsForEndpoint =
incrementalReportsQueue.get(endpoint);
- return reportsForEndpoint != null && !reportsForEndpoint.isEmpty();
- }
+ if (reportsForEndpoint != null && !reportsForEndpoint.isEmpty()) {
+ return true;
+ }
+ }
+ Map<String, AtomicBoolean> fullReports =
isFullReportReadyToBeSent.get(endpoint);
+ return fullReports != null && fullReports.entrySet().stream()
+ .anyMatch(report -> report.getValue().get() &&
type2Reports.get(report.getKey()).get() != null);
}
/**
```
Please cover a cap of one with all three full reports ready, plus the case
where the ready flags exist but the report values are still null.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/HeartbeatEndpointTask.java:
##########
@@ -102,6 +105,8 @@ public HeartbeatEndpointTask(EndpointStateMachine
rpcEndpoint,
HDDS_CONTAINER_ACTION_MAX_LIMIT_DEFAULT);
this.maxPipelineActionsPerHB = conf.getInt(HDDS_PIPELINE_ACTION_MAX_LIMIT,
HDDS_PIPELINE_ACTION_MAX_LIMIT_DEFAULT);
+ this.maxReportsPerHB = conf.getInt(HDDS_CONTAINER_REPORT_MAX_LIMIT,
Review Comment:
Please keep the configured limit positive. There is no range validation for
this new property: at `0`, `getAllAvailableReports()` removes nothing, but
queued ICRs keep causing immediate successful heartbeats with no reports; at
`-1`, `getIncrementalReports()` throws `IllegalArgumentException` before the
heartbeat RPC. These are operator configuration errors, but they currently
disable report delivery or heartbeat delivery instead of being bounded. The
existing `Math.max(1, ...)` pattern used below for `refreshThreshold` also
guarantees progress here:
```diff
---
a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/HeartbeatEndpointTask.java
+++
b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/states/endpoint/HeartbeatEndpointTask.java
@@ -105,8 +105,8 @@
HDDS_CONTAINER_ACTION_MAX_LIMIT_DEFAULT);
this.maxPipelineActionsPerHB =
conf.getInt(HDDS_PIPELINE_ACTION_MAX_LIMIT,
HDDS_PIPELINE_ACTION_MAX_LIMIT_DEFAULT);
- this.maxReportsPerHB = conf.getInt(HDDS_CONTAINER_REPORT_MAX_LIMIT,
- HDDS_CONTAINER_REPORT_MAX_LIMIT_DEFAULT);
+ this.maxReportsPerHB = Math.max(1,
conf.getInt(HDDS_CONTAINER_REPORT_MAX_LIMIT,
+ HDDS_CONTAINER_REPORT_MAX_LIMIT_DEFAULT));
if (versionManager != null) {
this.layoutVersionManager = versionManager;
} else {
```
Please cover zero and negative configured values.
--
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]
