devmadhuu commented on code in PR #10198:
URL: https://github.com/apache/ozone/pull/10198#discussion_r3199851443
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -812,4 +818,54 @@ public Response getOmContainersDeletedInSCM(
response.put("containerDiscrepancyInfo", containerDiscrepancyInfoList);
return Response.ok(response).build();
}
+
+ /**
+ * Return all containers in QUASI_CLOSED state.
+ *
+ * @param limit max no. of containers to get.
+ * @param prevKey the containerID after which results are returned.
+ * @return {@link Response}
+ */
+ @GET
+ @Path("/quasiClosed")
+ public Response getQuasiClosedContainers(
Review Comment:
Can we not use existing /unhealthy API endpoint, why need new API end point ?
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -812,4 +818,54 @@ public Response getOmContainersDeletedInSCM(
response.put("containerDiscrepancyInfo", containerDiscrepancyInfoList);
return Response.ok(response).build();
}
+
+ /**
+ * Return all containers in QUASI_CLOSED state.
+ *
+ * @param limit max no. of containers to get.
+ * @param prevKey the containerID after which results are returned.
+ * @return {@link Response}
+ */
+ @GET
+ @Path("/quasiClosed")
+ public Response getQuasiClosedContainers(
+ @DefaultValue(DEFAULT_FETCH_COUNT) @QueryParam(RECON_QUERY_LIMIT) int
limit,
+ @DefaultValue(PREV_CONTAINER_ID_DEFAULT_VALUE)
@QueryParam(RECON_QUERY_PREVKEY) long prevKey) {
Review Comment:
Also this new API endpoint uses `prevKey` as parameter, but frontend is
sending `minContainerId`, so API will always fallback to default value of
`prevKey` as 0 and pagination is broke here completely. Every `"next page"
`click will return the first page again.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -812,4 +818,54 @@ public Response getOmContainersDeletedInSCM(
response.put("containerDiscrepancyInfo", containerDiscrepancyInfoList);
return Response.ok(response).build();
}
+
+ /**
+ * Return all containers in QUASI_CLOSED state.
+ *
+ * @param limit max no. of containers to get.
+ * @param prevKey the containerID after which results are returned.
+ * @return {@link Response}
+ */
+ @GET
+ @Path("/quasiClosed")
+ public Response getQuasiClosedContainers(
+ @DefaultValue(DEFAULT_FETCH_COUNT) @QueryParam(RECON_QUERY_LIMIT) int
limit,
+ @DefaultValue(PREV_CONTAINER_ID_DEFAULT_VALUE)
@QueryParam(RECON_QUERY_PREVKEY) long prevKey) {
+
+ List<ContainerInfo> containers = containerManager.getContainers(
+ ContainerID.valueOf(prevKey + 1), limit,
HddsProtos.LifeCycleState.QUASI_CLOSED);
+
+ List<UnhealthyContainerMetadata> metaList = containers.stream()
+ .map(ci -> {
+ long containerID = ci.getContainerID();
+ int requiredNodes = 0;
+ try {
+ requiredNodes = ci.getReplicationConfig().getRequiredNodes();
+ } catch (Exception e) {
+ LOG.warn("Could not get required nodes for container {}",
containerID, e);
+ }
+ List<ContainerHistory> replicas =
containerManager.getLatestContainerHistory(containerID, requiredNodes);
+
+ UnhealthyContainerMetadata metadata = new UnhealthyContainerMetadata(
+ containerID,
+ "QUASI_CLOSED",
+ ci.getStateEnterTime() != null ?
ci.getStateEnterTime().toEpochMilli() : 0L,
+ requiredNodes,
+ replicas.size(),
+ replicas.size() - requiredNodes,
+ "",
+ ci.getNumberOfKeys(),
+ ci.getPipelineID() != null ? ci.getPipelineID().getId() : null,
+ replicas
+ );
+ return metadata;
+ })
+ .collect(Collectors.toList());
+
+ long firstKey = metaList.isEmpty() ? prevKey :
metaList.get(0).getContainerID();
+ long lastKey = metaList.isEmpty() ? prevKey :
metaList.get(metaList.size() - 1).getContainerID();
+ long total =
containerManager.getContainerStateCount(HddsProtos.LifeCycleState.QUASI_CLOSED);
+
+ return Response.ok(new QuasiClosedContainersResponse(total, firstKey,
lastKey, metaList)).build();
Review Comment:
Here, this `QuasiClosedContainersResponse` object is used to wrap the
response sent to frontend, but frontend uses `ContainersPaginationResponse` and
two new fields added `quasiClosedCount` and `totalCount` there, but don't see
totalCount field got added in backend response object -
`QuasiClosedContainersResponse`
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -812,4 +818,54 @@ public Response getOmContainersDeletedInSCM(
response.put("containerDiscrepancyInfo", containerDiscrepancyInfoList);
return Response.ok(response).build();
}
+
+ /**
+ * Return all containers in QUASI_CLOSED state.
+ *
+ * @param limit max no. of containers to get.
+ * @param prevKey the containerID after which results are returned.
+ * @return {@link Response}
+ */
+ @GET
+ @Path("/quasiClosed")
+ public Response getQuasiClosedContainers(
+ @DefaultValue(DEFAULT_FETCH_COUNT) @QueryParam(RECON_QUERY_LIMIT) int
limit,
+ @DefaultValue(PREV_CONTAINER_ID_DEFAULT_VALUE)
@QueryParam(RECON_QUERY_PREVKEY) long prevKey) {
+
+ List<ContainerInfo> containers = containerManager.getContainers(
+ ContainerID.valueOf(prevKey + 1), limit,
HddsProtos.LifeCycleState.QUASI_CLOSED);
+
+ List<UnhealthyContainerMetadata> metaList = containers.stream()
+ .map(ci -> {
+ long containerID = ci.getContainerID();
+ int requiredNodes = 0;
+ try {
+ requiredNodes = ci.getReplicationConfig().getRequiredNodes();
+ } catch (Exception e) {
+ LOG.warn("Could not get required nodes for container {}",
containerID, e);
+ }
+ List<ContainerHistory> replicas =
containerManager.getLatestContainerHistory(containerID, requiredNodes);
+
+ UnhealthyContainerMetadata metadata = new UnhealthyContainerMetadata(
Review Comment:
I would recommend to create a dedicated `QuasiClosedContainerMetadata` that
has fields like `stateEnterTime`, `replicaCount`, `replicaDetails`, else use of
existing `UnhealthyContainerMetadata` fields doesn't feel semantically correct
and confusing.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -812,4 +818,54 @@ public Response getOmContainersDeletedInSCM(
response.put("containerDiscrepancyInfo", containerDiscrepancyInfoList);
return Response.ok(response).build();
}
+
+ /**
+ * Return all containers in QUASI_CLOSED state.
+ *
+ * @param limit max no. of containers to get.
+ * @param prevKey the containerID after which results are returned.
+ * @return {@link Response}
+ */
+ @GET
+ @Path("/quasiClosed")
+ public Response getQuasiClosedContainers(
+ @DefaultValue(DEFAULT_FETCH_COUNT) @QueryParam(RECON_QUERY_LIMIT) int
limit,
+ @DefaultValue(PREV_CONTAINER_ID_DEFAULT_VALUE)
@QueryParam(RECON_QUERY_PREVKEY) long prevKey) {
+
+ List<ContainerInfo> containers = containerManager.getContainers(
+ ContainerID.valueOf(prevKey + 1), limit,
HddsProtos.LifeCycleState.QUASI_CLOSED);
+
+ List<UnhealthyContainerMetadata> metaList = containers.stream()
+ .map(ci -> {
+ long containerID = ci.getContainerID();
+ int requiredNodes = 0;
+ try {
+ requiredNodes = ci.getReplicationConfig().getRequiredNodes();
+ } catch (Exception e) {
+ LOG.warn("Could not get required nodes for container {}",
containerID, e);
+ }
+ List<ContainerHistory> replicas =
containerManager.getLatestContainerHistory(containerID, requiredNodes);
Review Comment:
We are calling this method inside a stream lambda and has no exception
handling for this method when called here. If it throws a runtime exception,
then entire response will fail. Better extract whole logic into a helper method
(like `toQuasiClosedMetadata`) with proper try/catch and
`WebApplicationException` wrapping. You can see how below is done:
```
private UnhealthyContainerMetadata toUnhealthyMetadata(
ContainerHealthSchemaManager.UnhealthyContainerRecord record) {
try {
...
} catch (IOException ioEx) {
throw new UncheckedIOException(ioEx);
}
}
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -447,6 +448,11 @@ private Response getUnhealthyContainersFromSchema(
for (UnhealthyContainersSummary s : summary) {
response.setSummaryCount(s.getContainerState(), s.getCount());
}
+
+ // Also include the quasi-closed count in the summary for the frontend
Highlights tab
+ long quasiClosedCount =
containerManager.getContainerStateCount(HddsProtos.LifeCycleState.QUASI_CLOSED);
Review Comment:
Actually this is not good to inject here, because above all calls are
fetching data from DB, so they are making DB call. But here, its a in-memory
data to be retrieved and if user just wants to see QUASI CLOSED containers, we
are simply making DB call also and retrieving all other unhealthy containers.
--
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]
