You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "priyeshkaratha (via GitHub)" <gi...@apache.org> on 2026/06/03 17:36:15 UTC
[PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
priyeshkaratha opened a new pull request, #10426:
URL: https://github.com/apache/ozone/pull/10426
## What changes were proposed in this pull request?
As part of this PR DataNodeMetricsServiceResponse is splitted into DataNodeMetricsCompleteResponse and DataNodeMetricsProgressResponse since during in progress state there is not meaning of including metric values there. Only need the status and message.
## What is the link to the Apache JIRA
HDDS-15452
## How was this patch tested?
Tested using updated test cases.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450134040
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -74,6 +75,7 @@ public class DataNodeMetricsService {
private final AtomicBoolean isRunning = new AtomicBoolean(false);
private MetricCollectionStatus currentStatus = MetricCollectionStatus.NOT_STARTED;
+ private String failedMessage = "Metrics collection task failed. Please retry after some time.";
Review Comment:
A suggestion from AI was that this is written from the async `collectMetrics` thread and read from the request thread in `getCollectedMetrics` - without synchronization.
In a concurrent scenario the reader could see a partially-constructed String reference (unlikely on x86, possible on ARM) or a stale value. Consider making it volatile, or bundling status+message in a single immutable snapshot object that is atomically swapped.
Since we do add support for ARM based processors, do you think this would be good to address?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on PR #10426:
URL: https://github.com/apache/ozone/pull/10426#issuecomment-4679375033
> @priyeshkaratha , I have two general questions,
>
> 1. after this change, does the front end UI code need change too?
> 2. what's the benefit or motivation of split into two DTOs?
Hi @ChenSammi,
The main motivation for splitting this into two DTOs is to ensure that each API returns a response that is relevant to its specific use case.
The UI is already compatible with these API changes and has been validated. With the current shared DTO, when the download report API is invoked, the response includes fields related to pending deletion metrics, which are not applicable to the download insights feature and therefore appear as `null` or `0`.
For example, the current response looks like:
```json
{
"status": "IN_PROGRESS",
"totalPendingDeletionSize": null,
"pendingDeletionPerDataNode": null,
"totalNodesQueried": 0,
"totalNodeQueriesFailed": 0
}
```
In this case, the response indicates that the operation is still in progress, but the remaining fields are either `null` or `0`, which do not provide meaningful information for the download insights workflow and can be confusing to users. Instead, it is preferable to return a response structure that is specific to the download feature and can convey an appropriate status or message.
By using separate DTOs, each API returns only the fields that are relevant to its functionality. This results in cleaner responses, clearer API semantics, and avoids exposing irrelevant `null` or default-valued fields in the output.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450166527
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/PendingDeletionEndpoint.java:
##########
@@ -87,12 +87,15 @@ private Response handleDataNodeMetrics(Integer limit) {
.entity("Limit query parameter must be at-least 1").build();
}
- DataNodeMetricsServiceResponse response = dataNodeMetricsService.getCollectedMetrics(limit);
- if (response.getStatus() == DataNodeMetricsService.MetricCollectionStatus.FINISHED) {
- return Response.ok(response).build();
- } else {
- return Response.accepted(response).build();
+ Object response = dataNodeMetricsService.getCollectedMetrics(limit);
+ if (response instanceof DataNodeMetricsCompleteResponse) {
+ DataNodeMetricsCompleteResponse completeResponse = (DataNodeMetricsCompleteResponse) response;
+ if (completeResponse.getStatus() == DataNodeMetricsService.MetricCollectionStatus.FINISHED) {
+ return Response.ok(completeResponse).build();
+ }
+ return Response.accepted(completeResponse).build();
Review Comment:
yes, makes sense
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3449753822
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -301,27 +302,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
- DataNodeMetricsServiceResponse.Builder dnMetricsBuilder = DataNodeMetricsServiceResponse.newBuilder();
- dnMetricsBuilder
- .setStatus(currentStatus)
- .setTotalPendingDeletionSize(totalPendingDeletion)
- .setTotalNodesQueried(totalNodesQueried)
- .setTotalNodeQueryFailures(totalNodesFailed);
+ List<DatanodePendingDeletionMetrics> list =
+ (limit == null) ? pendingDeletionList :
+ pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size()));
+ return new DataNodeMetricsCompleteResponse(
+ currentStatus,
+ totalNodesQueried,
+ totalNodesFailed,
+ totalPendingDeletion,
+ list);
+ }
- if (null == limit) {
- return dnMetricsBuilder.setPendingDeletion(pendingDeletionList).build();
- } else {
- return dnMetricsBuilder.setPendingDeletion(
- pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size())
- )).build();
- }
+ return new DataNodeMetricsProgressResponse(
+ currentStatus,
+ buildProgressMessage(currentStatus));
+ }
+
+ private static String buildProgressMessage(MetricCollectionStatus status) {
+ switch (status) {
+ case IN_PROGRESS:
+ return "Metrics collection task is currently running. Please wait for task to finish.";
+ case FAILED:
Review Comment:
Addressed the changes.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450210504
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/DataNodeMetricsCompleteResponse.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.recon.api.types;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.List;
+import org.apache.hadoop.ozone.recon.api.DataNodeMetricsService;
+
+/**
+ * Response returned when metrics collection is complete.
+ * Includes the metric payload fields.
+ */
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class DataNodeMetricsCompleteResponse {
+
+ @JsonProperty("status")
+ private final DataNodeMetricsService.MetricCollectionStatus status;
+
+ @JsonProperty("totalNodesQueried")
+ private final int totalNodesQueried;
+
+ @JsonProperty("totalNodeQueriesFailed")
+ private final long totalNodeQueryFailures;
+
+ @JsonProperty("totalPendingDeletionSize")
+ private final Long totalPendingDeletionSize;
+
+ @JsonProperty("pendingDeletionPerDataNode")
+ private final List<DatanodePendingDeletionMetrics> pendingDeletionPerDataNode;
Review Comment:
Better to remove NON_NULL annotation since we have already separate DTO
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3400090853
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -299,27 +300,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
Review Comment:
This is the old way of implementation. But it is actually confusing the user with null and zero 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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3418035358
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -301,27 +302,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
- DataNodeMetricsServiceResponse.Builder dnMetricsBuilder = DataNodeMetricsServiceResponse.newBuilder();
- dnMetricsBuilder
- .setStatus(currentStatus)
- .setTotalPendingDeletionSize(totalPendingDeletion)
- .setTotalNodesQueried(totalNodesQueried)
- .setTotalNodeQueryFailures(totalNodesFailed);
+ List<DatanodePendingDeletionMetrics> list =
+ (limit == null) ? pendingDeletionList :
+ pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size()));
+ return new DataNodeMetricsCompleteResponse(
+ currentStatus,
+ totalNodesQueried,
+ totalNodesFailed,
+ totalPendingDeletion,
+ list);
+ }
- if (null == limit) {
- return dnMetricsBuilder.setPendingDeletion(pendingDeletionList).build();
- } else {
- return dnMetricsBuilder.setPendingDeletion(
- pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size())
- )).build();
- }
+ return new DataNodeMetricsProgressResponse(
+ currentStatus,
+ buildProgressMessage(currentStatus));
+ }
+
+ private static String buildProgressMessage(MetricCollectionStatus status) {
+ switch (status) {
+ case IN_PROGRESS:
+ return "Metrics collection task is currently running. Please wait for task to finish.";
+ case FAILED:
Review Comment:
@ChenSammi Currently, the UI only checks whether the operation status is FINISHED. If the status is anything other than FINISHED, the UI continues polling, which means it will also retry in cases where the operation has FAILED.
At the moment, we are not propagating the exact failure reason back to the UI, as supporting that would require additional handling on both the backend and UI sides. I can address this enhancement as part of a separate JIRA in next phase.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3386842393
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/DataNodeMetricsProgressResponse.java:
##########
@@ -0,0 +1,51 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.recon.api.types;
+
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.hadoop.ozone.recon.api.DataNodeMetricsService;
+
+/**
+ * Response returned while metrics collection is still in progress.
+ * Intentionally omits metric payload fields.
+ */
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class DataNodeMetricsProgressResponse {
+
+ @JsonProperty("status")
+ private final DataNodeMetricsService.MetricCollectionStatus status;
+
+ @JsonProperty("message")
+ private final String message;
+
+ public DataNodeMetricsProgressResponse(
+ DataNodeMetricsService.MetricCollectionStatus status,
+ String message) {
+ this.status = status;
+ this.message = message;
+ }
Review Comment:
DataNodeMetricsProgressResponse currently has only a parameterized constructor with un-annotated parameters and final fields. Jackson deserialization will be unreliable (and typically fails unless compiled with `-parameters` or using special mapper features), which makes it hard for clients/tests to parse the IN_PROGRESS/FAILED JSON response. Consider annotating the constructor with `@JsonCreator` and its parameters with `@JsonProperty` to make the DTO safely deserializable.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -299,27 +300,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
Review Comment:
Returning `Object` from getCollectedMetrics() forces callers to use `instanceof`/casts and obscures the API contract. Consider introducing a shared supertype (eg `DataNodeMetricsResponse` interface/base class exposing `getStatus()` and optional `getMessage()`), returning that from the service, and updating endpoints accordingly; it keeps type-safety while still allowing state-specific DTOs.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/PendingDeletionEndpoint.java:
##########
@@ -87,12 +87,11 @@ private Response handleDataNodeMetrics(Integer limit) {
.entity("Limit query parameter must be at-least 1").build();
}
- DataNodeMetricsServiceResponse response = dataNodeMetricsService.getCollectedMetrics(limit);
- if (response.getStatus() == DataNodeMetricsService.MetricCollectionStatus.FINISHED) {
+ Object response = dataNodeMetricsService.getCollectedMetrics(limit);
+ if (response instanceof DataNodeMetricsCompleteResponse) {
return Response.ok(response).build();
- } else {
- return Response.accepted(response).build();
}
+ return Response.accepted(response).build();
Review Comment:
handleDataNodeMetrics() now returns HTTP 200 solely based on `response instanceof DataNodeMetricsCompleteResponse`. That makes the HTTP status dependent on DTO type rather than the actual collection state and can mask inconsistent/mocked states (eg a complete DTO with NOT_STARTED/FAILED status). Consider checking `getStatus() == FINISHED` before returning 200 OK.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/StorageDistributionEndpoint.java:
##########
@@ -169,18 +169,19 @@ public Response getStorageDistribution() {
@Path("/download")
public Response downloadDataNodeStorageDistribution() {
- DataNodeMetricsServiceResponse metricsResponse =
- dataNodeMetricsService.getCollectedMetrics(null);
+ Object metricsResponse = dataNodeMetricsService.getCollectedMetrics(null);
- if (metricsResponse.getStatus() != DataNodeMetricsService.MetricCollectionStatus.FINISHED) {
+ if (!(metricsResponse instanceof DataNodeMetricsCompleteResponse)) {
return Response.status(Response.Status.ACCEPTED)
.entity(metricsResponse)
Review Comment:
The endpoint now decides whether to return HTTP 202 vs proceed by checking the concrete DTO type (`instanceof DataNodeMetricsCompleteResponse`). This couples control-flow to class selection and can produce incorrect behavior if a complete DTO is ever returned with a non-FINISHED status (eg from a mock or future refactor). It’s safer to also validate `status == FINISHED` before treating the payload as complete.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3418035358
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -301,27 +302,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
- DataNodeMetricsServiceResponse.Builder dnMetricsBuilder = DataNodeMetricsServiceResponse.newBuilder();
- dnMetricsBuilder
- .setStatus(currentStatus)
- .setTotalPendingDeletionSize(totalPendingDeletion)
- .setTotalNodesQueried(totalNodesQueried)
- .setTotalNodeQueryFailures(totalNodesFailed);
+ List<DatanodePendingDeletionMetrics> list =
+ (limit == null) ? pendingDeletionList :
+ pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size()));
+ return new DataNodeMetricsCompleteResponse(
+ currentStatus,
+ totalNodesQueried,
+ totalNodesFailed,
+ totalPendingDeletion,
+ list);
+ }
- if (null == limit) {
- return dnMetricsBuilder.setPendingDeletion(pendingDeletionList).build();
- } else {
- return dnMetricsBuilder.setPendingDeletion(
- pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size())
- )).build();
- }
+ return new DataNodeMetricsProgressResponse(
+ currentStatus,
+ buildProgressMessage(currentStatus));
+ }
+
+ private static String buildProgressMessage(MetricCollectionStatus status) {
+ switch (status) {
+ case IN_PROGRESS:
+ return "Metrics collection task is currently running. Please wait for task to finish.";
+ case FAILED:
Review Comment:
@ChenSammi Currently, the UI only checks whether the operation status is FINISHED. If the status is anything other than FINISHED, the UI continues polling, which means it will also retry in cases where the operation has FAILED.
At the moment, we are not propagating the exact failure reason back to the UI, as supporting that would require additional handling on both the backend and UI sides. I can address this enhancement as part of a separate JIRA.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450123897
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
Review Comment:
This is possibly dead-code? Since we have removed the rate limit check.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450217982
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -117,12 +119,7 @@ public void startTask() {
return;
}
- // Check rate limit
- if (System.currentTimeMillis() - lastCollectionEndTime.get() < minimumApiDelayMs) {
- LOG.debug("Rate limit active, skipping collection (delay: {}ms)", minimumApiDelayMs);
- isRunning.set(false);
- return;
- }
Review Comment:
It removed mistakenly while testing the changes. Added it back
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
Review Comment:
added ratelimit check
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450149688
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -301,27 +299,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
- DataNodeMetricsServiceResponse.Builder dnMetricsBuilder = DataNodeMetricsServiceResponse.newBuilder();
- dnMetricsBuilder
- .setStatus(currentStatus)
- .setTotalPendingDeletionSize(totalPendingDeletion)
- .setTotalNodesQueried(totalNodesQueried)
- .setTotalNodeQueryFailures(totalNodesFailed);
+ List<DatanodePendingDeletionMetrics> list =
+ (limit == null) ? pendingDeletionList :
+ pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size()));
+ return new DataNodeMetricsCompleteResponse(
+ currentStatus,
+ totalNodesQueried,
+ totalNodesFailed,
+ totalPendingDeletion,
+ list);
+ }
- if (null == limit) {
- return dnMetricsBuilder.setPendingDeletion(pendingDeletionList).build();
- } else {
- return dnMetricsBuilder.setPendingDeletion(
- pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size())
- )).build();
- }
+ return new DataNodeMetricsProgressResponse(
+ currentStatus,
+ buildProgressMessage(currentStatus, failedMessage));
+ }
+
+ private static String buildProgressMessage(MetricCollectionStatus status, String failedMessage) {
+ switch (status) {
+ case IN_PROGRESS:
Review Comment:
Nit: I think checkstyle expects `case` to be indented inside instead of the same level.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450122733
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -117,12 +119,7 @@ public void startTask() {
return;
}
- // Check rate limit
- if (System.currentTimeMillis() - lastCollectionEndTime.get() < minimumApiDelayMs) {
- LOG.debug("Rate limit active, skipping collection (delay: {}ms)", minimumApiDelayMs);
- isRunning.set(false);
- return;
- }
Review Comment:
Is there a reason we are removing the rate limit check?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450170454
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/types/DataNodeMetricsCompleteResponse.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.recon.api.types;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonInclude;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import java.util.List;
+import org.apache.hadoop.ozone.recon.api.DataNodeMetricsService;
+
+/**
+ * Response returned when metrics collection is complete.
+ * Includes the metric payload fields.
+ */
+@JsonInclude(JsonInclude.Include.NON_NULL)
+public class DataNodeMetricsCompleteResponse {
+
+ @JsonProperty("status")
+ private final DataNodeMetricsService.MetricCollectionStatus status;
+
+ @JsonProperty("totalNodesQueried")
+ private final int totalNodesQueried;
+
+ @JsonProperty("totalNodeQueriesFailed")
+ private final long totalNodeQueryFailures;
+
+ @JsonProperty("totalPendingDeletionSize")
+ private final Long totalPendingDeletionSize;
+
+ @JsonProperty("pendingDeletionPerDataNode")
+ private final List<DatanodePendingDeletionMetrics> pendingDeletionPerDataNode;
Review Comment:
Since the annotation only includes NON_NULL fields, is it possible that this can get omitted?
I feel like the complete response should always have all fields, so it might be better to initialize this to an empty list, unless we are okay with having missing fields.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450341335
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -74,6 +75,7 @@ public class DataNodeMetricsService {
private final AtomicBoolean isRunning = new AtomicBoolean(false);
private MetricCollectionStatus currentStatus = MetricCollectionStatus.NOT_STARTED;
+ private String failedMessage = "Metrics collection task failed. Please retry after some time.";
Review Comment:
make sense.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450143988
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/PendingDeletionEndpoint.java:
##########
@@ -87,12 +87,15 @@ private Response handleDataNodeMetrics(Integer limit) {
.entity("Limit query parameter must be at-least 1").build();
}
- DataNodeMetricsServiceResponse response = dataNodeMetricsService.getCollectedMetrics(limit);
- if (response.getStatus() == DataNodeMetricsService.MetricCollectionStatus.FINISHED) {
- return Response.ok(response).build();
- } else {
- return Response.accepted(response).build();
+ Object response = dataNodeMetricsService.getCollectedMetrics(limit);
+ if (response instanceof DataNodeMetricsCompleteResponse) {
+ DataNodeMetricsCompleteResponse completeResponse = (DataNodeMetricsCompleteResponse) response;
+ if (completeResponse.getStatus() == DataNodeMetricsService.MetricCollectionStatus.FINISHED) {
+ return Response.ok(completeResponse).build();
+ }
+ return Response.accepted(completeResponse).build();
Review Comment:
Possible dead code? It seems like getCollectedMetrics() would only return type `DataNodeMetricsCompleteResponse` when `currentStatus == FINISHED`.
So the inner if is always true.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450319800
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -74,6 +75,7 @@ public class DataNodeMetricsService {
private final AtomicBoolean isRunning = new AtomicBoolean(false);
private MetricCollectionStatus currentStatus = MetricCollectionStatus.NOT_STARTED;
+ private String failedMessage = "Metrics collection task failed. Please retry after some time.";
Review Comment:
But scenario is like:
- Thread A (async `collectMetrics`) writes `failedMessage = e.getLocalizedMessage()` at line 165
- Thread B (request thread) reads `failedMessage` at line 327 via `getCollectedMetrics()`
It seems on ARM (which Ozone supports), Thread B could read a stale or partially-constructed value.
We could try making `failedMessage` volatile.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3418196037
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -301,27 +302,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
- DataNodeMetricsServiceResponse.Builder dnMetricsBuilder = DataNodeMetricsServiceResponse.newBuilder();
- dnMetricsBuilder
- .setStatus(currentStatus)
- .setTotalPendingDeletionSize(totalPendingDeletion)
- .setTotalNodesQueried(totalNodesQueried)
- .setTotalNodeQueryFailures(totalNodesFailed);
+ List<DatanodePendingDeletionMetrics> list =
+ (limit == null) ? pendingDeletionList :
+ pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size()));
+ return new DataNodeMetricsCompleteResponse(
+ currentStatus,
+ totalNodesQueried,
+ totalNodesFailed,
+ totalPendingDeletion,
+ list);
+ }
- if (null == limit) {
- return dnMetricsBuilder.setPendingDeletion(pendingDeletionList).build();
- } else {
- return dnMetricsBuilder.setPendingDeletion(
- pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size())
- )).build();
- }
+ return new DataNodeMetricsProgressResponse(
+ currentStatus,
+ buildProgressMessage(currentStatus));
+ }
+
+ private static String buildProgressMessage(MetricCollectionStatus status) {
+ switch (status) {
+ case IN_PROGRESS:
+ return "Metrics collection task is currently running. Please wait for task to finish.";
+ case FAILED:
Review Comment:
Despite the UI behavior, this restfull API can also be called by user's own tool, can we return a more clear failure message than ""Metrics collection task failed. Please retry after some time."?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #10426:
URL: https://github.com/apache/ozone/pull/10426#issuecomment-4778899456
Thanks @priyeshkaratha , and @spacemonkd for the review.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on PR #10426:
URL: https://github.com/apache/ozone/pull/10426#issuecomment-4765563430
@spacemonkd Thanks for the review and addressed review comments.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "spacemonkd (via GitHub)" <gi...@apache.org>.
spacemonkd commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450159427
##########
hadoop-ozone/integration-test-recon/src/test/java/org/apache/hadoop/ozone/recon/AbstractTestStorageDistributionEndpoint.java:
##########
@@ -313,8 +313,8 @@ protected boolean verifyPendingDeletionAfterKeyDeletionDn() {
StringBuilder urlBuilder = new StringBuilder();
urlBuilder.append(getReconWebAddress(conf)).append(PENDING_DELETION_ENDPOINT).append("?component=dn");
String response = TestReconEndpointUtil.makeHttpCall(conf, urlBuilder);
- DataNodeMetricsServiceResponse pendingDeletion =
- MAPPER.readValue(response, DataNodeMetricsServiceResponse.class);
+ DataNodeMetricsCompleteResponse pendingDeletion =
Review Comment:
Do you think there can be a race condition where collection hasn't finished when the test calls, as a result it gets `DataNodeMetricsProgressResponse`?
Then this would possibly be flaky. It might be better to either:
- retry until status is FINISHED
- or assert on the JSON status
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450151342
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -74,6 +75,7 @@ public class DataNodeMetricsService {
private final AtomicBoolean isRunning = new AtomicBoolean(false);
private MetricCollectionStatus currentStatus = MetricCollectionStatus.NOT_STARTED;
+ private String failedMessage = "Metrics collection task failed. Please retry after some time.";
Review Comment:
Since we have rate limit, this is fine.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3411408670
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/DataNodeMetricsService.java:
##########
@@ -301,27 +302,40 @@ private void resetState() {
totalNodesFailed = 0;
}
- public DataNodeMetricsServiceResponse getCollectedMetrics(Integer limit) {
+ /**
+ * Returns either {@link DataNodeMetricsCompleteResponse} when collection is
+ * finished, or {@link DataNodeMetricsProgressResponse} otherwise.
+ */
+ public Object getCollectedMetrics(Integer limit) {
startTask();
if (currentStatus == MetricCollectionStatus.FINISHED) {
- DataNodeMetricsServiceResponse.Builder dnMetricsBuilder = DataNodeMetricsServiceResponse.newBuilder();
- dnMetricsBuilder
- .setStatus(currentStatus)
- .setTotalPendingDeletionSize(totalPendingDeletion)
- .setTotalNodesQueried(totalNodesQueried)
- .setTotalNodeQueryFailures(totalNodesFailed);
+ List<DatanodePendingDeletionMetrics> list =
+ (limit == null) ? pendingDeletionList :
+ pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size()));
+ return new DataNodeMetricsCompleteResponse(
+ currentStatus,
+ totalNodesQueried,
+ totalNodesFailed,
+ totalPendingDeletion,
+ list);
+ }
- if (null == limit) {
- return dnMetricsBuilder.setPendingDeletion(pendingDeletionList).build();
- } else {
- return dnMetricsBuilder.setPendingDeletion(
- pendingDeletionList.subList(0, Math.min(limit, pendingDeletionList.size())
- )).build();
- }
+ return new DataNodeMetricsProgressResponse(
+ currentStatus,
+ buildProgressMessage(currentStatus));
+ }
+
+ private static String buildProgressMessage(MetricCollectionStatus status) {
+ switch (status) {
+ case IN_PROGRESS:
+ return "Metrics collection task is currently running. Please wait for task to finish.";
+ case FAILED:
Review Comment:
Will the metrics collection task auto retry itself? If no, then it's doesn't make sense to category FAILED as in progress state.
Do we reply the failure reason to API caller?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi commented on PR #10426:
URL: https://github.com/apache/ozone/pull/10426#issuecomment-4668220139
@priyeshkaratha , after this change, does the front end UI code need change too?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "priyeshkaratha (via GitHub)" <gi...@apache.org>.
priyeshkaratha commented on code in PR #10426:
URL: https://github.com/apache/ozone/pull/10426#discussion_r3450180710
##########
hadoop-ozone/integration-test-recon/src/test/java/org/apache/hadoop/ozone/recon/AbstractTestStorageDistributionEndpoint.java:
##########
@@ -313,8 +313,8 @@ protected boolean verifyPendingDeletionAfterKeyDeletionDn() {
StringBuilder urlBuilder = new StringBuilder();
urlBuilder.append(getReconWebAddress(conf)).append(PENDING_DELETION_ENDPOINT).append("?component=dn");
String response = TestReconEndpointUtil.makeHttpCall(conf, urlBuilder);
- DataNodeMetricsServiceResponse pendingDeletion =
- MAPPER.readValue(response, DataNodeMetricsServiceResponse.class);
+ DataNodeMetricsCompleteResponse pendingDeletion =
Review Comment:
yes, It will retry untill finished since mapper will throw error and catch is returning false.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-15452. Split DataNodeMetricsServiceResponse response into state specific DTOs [ozone]
Posted by "ChenSammi (via GitHub)" <gi...@apache.org>.
ChenSammi merged PR #10426:
URL: https://github.com/apache/ozone/pull/10426
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org