[GitHub] [ozone] fapifta commented on a diff in pull request #4129: HDDS-7650. Recon - Data Management Metrics - Understand the Age of Data.

["fapifta (via GitHub)" <gi...@apache.org>]

fapifta commented on code in PR #4129:
URL: https://github.com/apache/ozone/pull/4129#discussion_r1084756443


##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NSSummaryEndpoint.java:
##########
@@ -48,6 +56,8 @@
 @AdminOnly
 public class NSSummaryEndpoint {
 
+  public static final String COUNT = "count";

Review Comment:
   Shouldn't we have createTime here? Also shouldn't that be cTime or creationTime?
   As these three belongs together, wouldn't it worth it to create an enum for the values we accept, and validate and parse the parameters based on the enum?



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NSSummaryEndpoint.java:
##########
@@ -116,21 +294,49 @@ public Response getDiskUsage(@QueryParam("path") String path,
     if (path == null || path.length() == 0) {
       return Response.status(Response.Status.BAD_REQUEST).build();
     }
+    return Response.ok(getDuResponse(path, listFile, withReplica)).build();
+  }
 
+  private DUResponse getDuResponse(String path, boolean listFile,
+                                   boolean withReplica) throws IOException {
+    AtomicBoolean isError = new AtomicBoolean(false);

Review Comment:
   I believe we can use a simple boolean here, even if the forEach runs in parallel mode, the only change to the value is to set it to true, and we just check after execution is done, so we don't need atomicity on it as it does not matter i one or 3k threads sets it to true at once, but correct me if I am wrong.



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NSSummaryEndpoint.java:
##########
@@ -116,21 +294,49 @@ public Response getDiskUsage(@QueryParam("path") String path,
     if (path == null || path.length() == 0) {
       return Response.status(Response.Status.BAD_REQUEST).build();
     }
+    return Response.ok(getDuResponse(path, listFile, withReplica)).build();

Review Comment:
   After extracting the method here, but even before that, we can remove the @SuppressWarnings for the /du endpoint method in line 287



##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NSSummaryEndpoint.java:
##########
@@ -62,6 +72,172 @@ public NSSummaryEndpoint(ReconNamespaceSummaryManager namespaceSummaryManager,
     this.reconSCM = reconSCM;
   }
 
+  @GET
+  @Path("/entityinfo")
+  public Response getEntityMetrics(@QueryParam("path") String path,

Review Comment:
   Can we add API documentation for this method if we keep it?



-- 
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