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: [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]
