Copilot commented on code in PR #9994:
URL: https://github.com/apache/ozone/pull/9994#discussion_r3076552161
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -450,6 +459,71 @@ private Response getUnhealthyContainersFromSchema(
return Response.ok(response).build();
}
+ /**
+ * Export all unhealthy containers into a CSV file by streaming the results
directly
+ * from the database without holding them in the JVM heap.
+ *
+ * @param state The container state to filter by, or null for all.
+ * @param limit The maximum number of records to return, 0 for unlimited.
+ * @return {@link Response} containing the CSV StreamingOutput.
+ */
+ @GET
+ @Path("/unhealthy/export")
+ @Produces("text/csv")
+ public Response exportUnhealthyContainers(
+ @QueryParam("state") String state,
+ @DefaultValue("0") @QueryParam(RECON_QUERY_LIMIT) int limit) {
+
+ ContainerSchemaDefinition.UnHealthyContainerStates internalState = null;
+ if (StringUtils.isNotEmpty(state)) {
+ try {
+ internalState =
ContainerSchemaDefinition.UnHealthyContainerStates.valueOf(state);
+ } catch (IllegalArgumentException e) {
+ throw new WebApplicationException(e, Response.Status.BAD_REQUEST);
+ }
+ }
+
+ final ContainerSchemaDefinition.UnHealthyContainerStates filterState =
internalState;
+
+ StreamingOutput stream = outputStream -> {
+ try (BufferedOutputStream bos = new BufferedOutputStream(outputStream,
256 * 1024);
+ Cursor<UnhealthyContainersRecord> cursor =
+
containerHealthSchemaManager.getUnhealthyContainersCursor(filterState, limit)) {
+
+ PrintWriter writer = new PrintWriter(new OutputStreamWriter(bos,
StandardCharsets.UTF_8));
+
+ // Write CSV header
+ writer.println("container_id,container_state,in_state_since," +
+ "expected_replica_count,actual_replica_count,replica_delta");
+
+ StringBuilder sb = new StringBuilder(128);
+ while (cursor.hasNext()) {
+ UnhealthyContainersRecord rec = cursor.fetchNext();
+ sb.setLength(0);
+ sb.append(rec.getContainerId()).append(',')
+ .append(rec.getContainerState()).append(',')
+ .append(rec.getInStateSince()).append(',')
+ .append(rec.getExpectedReplicaCount()).append(',')
+ .append(rec.getActualReplicaCount()).append(',')
+ .append(rec.getReplicaDelta());
+ writer.println(sb);
+ }
+ writer.flush();
+ } catch (Exception e) {
+ LOG.error("Error streaming unhealthy containers CSV", e);
+ throw new WebApplicationException(e,
Response.Status.INTERNAL_SERVER_ERROR);
+ }
+ };
+
+ String filename = String.format("unhealthy_containers_%s_%d.csv",
+ state != null ? state.toLowerCase() : "all",
+ System.currentTimeMillis());
+
+ return Response.ok(stream)
+ .header("Content-Disposition", "attachment; filename=\"" + filename +
"\"")
+ .build();
+ }
Review Comment:
This new export behavior isn’t covered by tests. `TestContainerEndpoint`
already has coverage for unhealthy container APIs; please add tests for
`/containers/unhealthy/export` to verify: (1) valid state streams CSV with a
header, (2) invalid state returns 400, and (3) the configured max-records cap
is enforced/clamps the requested limit.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -450,6 +459,71 @@ private Response getUnhealthyContainersFromSchema(
return Response.ok(response).build();
}
+ /**
+ * Export all unhealthy containers into a CSV file by streaming the results
directly
+ * from the database without holding them in the JVM heap.
+ *
+ * @param state The container state to filter by, or null for all.
+ * @param limit The maximum number of records to return, 0 for unlimited.
+ * @return {@link Response} containing the CSV StreamingOutput.
+ */
+ @GET
+ @Path("/unhealthy/export")
+ @Produces("text/csv")
+ public Response exportUnhealthyContainers(
+ @QueryParam("state") String state,
+ @DefaultValue("0") @QueryParam(RECON_QUERY_LIMIT) int limit) {
+
Review Comment:
The `limit` query param is not validated here: negative values currently
behave like “unlimited” (because the cursor query only applies a limit when
`limit > 0`). Please reject `limit < 0` with 406/400 (consistent with other
endpoints) so clients can’t accidentally trigger unbounded exports.
```suggestion
if (limit < 0) {
throw new WebApplicationException("The limit query parameter must be "
+ "greater than or equal to 0.", Response.Status.BAD_REQUEST);
}
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -450,6 +459,71 @@ private Response getUnhealthyContainersFromSchema(
return Response.ok(response).build();
}
+ /**
+ * Export all unhealthy containers into a CSV file by streaming the results
directly
+ * from the database without holding them in the JVM heap.
+ *
+ * @param state The container state to filter by, or null for all.
+ * @param limit The maximum number of records to return, 0 for unlimited.
+ * @return {@link Response} containing the CSV StreamingOutput.
+ */
+ @GET
+ @Path("/unhealthy/export")
+ @Produces("text/csv")
+ public Response exportUnhealthyContainers(
+ @QueryParam("state") String state,
+ @DefaultValue("0") @QueryParam(RECON_QUERY_LIMIT) int limit) {
+
+ ContainerSchemaDefinition.UnHealthyContainerStates internalState = null;
+ if (StringUtils.isNotEmpty(state)) {
+ try {
+ internalState =
ContainerSchemaDefinition.UnHealthyContainerStates.valueOf(state);
+ } catch (IllegalArgumentException e) {
+ throw new WebApplicationException(e, Response.Status.BAD_REQUEST);
+ }
+ }
+
+ final ContainerSchemaDefinition.UnHealthyContainerStates filterState =
internalState;
+
+ StreamingOutput stream = outputStream -> {
+ try (BufferedOutputStream bos = new BufferedOutputStream(outputStream,
256 * 1024);
+ Cursor<UnhealthyContainersRecord> cursor =
+
containerHealthSchemaManager.getUnhealthyContainersCursor(filterState, limit)) {
+
+ PrintWriter writer = new PrintWriter(new OutputStreamWriter(bos,
StandardCharsets.UTF_8));
+
+ // Write CSV header
+ writer.println("container_id,container_state,in_state_since," +
+ "expected_replica_count,actual_replica_count,replica_delta");
+
+ StringBuilder sb = new StringBuilder(128);
+ while (cursor.hasNext()) {
+ UnhealthyContainersRecord rec = cursor.fetchNext();
+ sb.setLength(0);
+ sb.append(rec.getContainerId()).append(',')
+ .append(rec.getContainerState()).append(',')
+ .append(rec.getInStateSince()).append(',')
+ .append(rec.getExpectedReplicaCount()).append(',')
+ .append(rec.getActualReplicaCount()).append(',')
+ .append(rec.getReplicaDelta());
+ writer.println(sb);
+ }
+ writer.flush();
Review Comment:
`PrintWriter` swallows IOExceptions (it sets an internal error flag instead
of throwing). For a long-running stream, this can hide client disconnects /
broken pipes and make failures harder to detect. Consider using a
`BufferedWriter`/`OutputStream` that propagates IOExceptions (or at least check
`writer.checkError()` periodically) so streaming failures surface reliably.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/persistence/ContainerHealthSchemaManager.java:
##########
@@ -381,6 +382,42 @@ public List<UnhealthyContainerRecord>
getUnhealthyContainers(
}
}
+ /**
+ * Returns a streaming cursor over unhealthy container records.
+ * Caller MUST close the cursor.
+ *
+ * @param state filter by state, or null for all states
+ * @param limit max records to return, 0 = unlimited
+ * @return Cursor returning UnhealthyContainersRecord
+ */
+ public Cursor<UnhealthyContainersRecord> getUnhealthyContainersCursor(
+ UnHealthyContainerStates state, int limit) {
+ DSLContext dslContext = containerSchemaDefinition.getDSLContext();
+ org.jooq.SelectQuery<UnhealthyContainersRecord> query =
dslContext.selectFrom(UNHEALTHY_CONTAINERS).getQuery();
+
+ if (state != null) {
+
query.addConditions(UNHEALTHY_CONTAINERS.CONTAINER_STATE.eq(state.toString()));
+ // Single-state filter: ordering by container_state is redundant (every
+ // row has the same value). ORDER BY container_id alone lets Derby walk
+ // the composite index idx_state_container_id in order and return rows
+ // immediately — no sort step, eliminating "time to first byte" delay.
+ query.addOrderBy(UNHEALTHY_CONTAINERS.CONTAINER_ID.asc());
+ } else {
+ // All-states query: order by state first, then container_id. This
+ // matches the composite index order so Derby can still avoid a sort.
+ query.addOrderBy(
+ UNHEALTHY_CONTAINERS.CONTAINER_STATE.asc(),
+ UNHEALTHY_CONTAINERS.CONTAINER_ID.asc()
+ );
+ }
+
+ if (limit > 0) {
+ query.addLimit(limit);
+ }
+
+ return query.fetchLazy();
Review Comment:
`getUnhealthyContainersCursor` streams via `fetchLazy()`, but unlike
`getUnhealthyContainers(...)` it doesn’t set a JDBC fetch size. Without it,
Derby/JDBC may default to very small batches, significantly increasing
round-trips for million-row exports. Consider setting a reasonable
`query.fetchSize(...)` (possibly configurable) for better streaming throughput.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServerConfigKeys.java:
##########
@@ -253,6 +253,7 @@ public final class ReconServerConfigKeys {
"ozone.recon.scm.container.id.batch.size";
public static final long OZONE_RECON_SCM_CONTAINER_ID_BATCH_SIZE_DEFAULT =
1_000_000;
+
Review Comment:
The PR description mentions a new config key
`ozone.recon.csv.export.max.records`, but no constant (or any config key) is
added here—this change is currently just a blank line. Please add the config
key + default (eg 5,000,000) to `ReconServerConfigKeys` and use it in the
export endpoint to enforce a hard cap.
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/pages/containers/containers.tsx:
##########
@@ -175,6 +178,31 @@ const Containers: React.FC<{}> = () => {
5: mismatchedReplicaContainerData
}
+ // Mapping tab keys to the backend state parameter for CSV export
+ const tabToExportState: Record<string, string> = {
+ '1': 'MISSING',
+ '2': 'UNDER_REPLICATED',
+ '3': 'OVER_REPLICATED',
+ '4': 'MIS_REPLICATED',
+ '5': 'REPLICA_MISMATCH'
+ };
+
+ // Human-readable labels for the export tooltip
+ const tabToLabel: Record<string, string> = {
+ '1': 'Missing',
+ '2': 'Under-Replicated',
+ '3': 'Over-Replicated',
+ '4': 'Mis-Replicated',
+ '5': 'Mismatched Replicas'
+ };
+
+ const handleExportCsv = useCallback(() => {
+ const state = tabToExportState[selectedTab];
+ const exportUrl =
`/api/v1/containers/unhealthy/export?state=${state}&limit=${exportLimit}`;
+ window.open(exportUrl, '_blank');
+ message.success(`Exporting ${tabToLabel[selectedTab]} containers as CSV
(Limit: ${exportLimit})`);
+ }, [selectedTab, exportLimit]);
+
Review Comment:
`handleExportCsv` always includes `limit=${exportLimit}` in the URL. With
the current UI default/choices, this can emit `limit=0` (Complete) and trigger
an unbounded server-side export. Once the backend enforces a max, it’d be safer
for the UI to omit the `limit` param when “Complete” is selected (letting the
server apply the configured max) and to avoid showing a success toast until the
download actually starts (eg, after a HEAD/check request).
```suggestion
const handleExportCsv = useCallback(async () => {
const state = tabToExportState[selectedTab];
const queryParams = new URLSearchParams({ state });
if (exportLimit > 0) {
queryParams.set('limit', exportLimit.toString());
}
const exportUrl =
`/api/v1/containers/unhealthy/export?${queryParams.toString()}`;
try {
const response = await fetch(exportUrl, { method: 'HEAD' });
if (!response.ok) {
throw new Error(`Failed to export containers. HTTP status:
${response.status}`);
}
window.open(exportUrl, '_blank');
message.success(
exportLimit > 0
? `Exporting ${tabToLabel[selectedTab]} containers as CSV (Limit:
${exportLimit})`
: `Exporting ${tabToLabel[selectedTab]} containers as CSV`
);
} catch {
message.error(`Failed to start CSV export for
${tabToLabel[selectedTab]} containers`);
}
}, [selectedTab, exportLimit, tabToExportState, tabToLabel]);
```
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -61,6 +67,7 @@
import org.apache.hadoop.ozone.om.helpers.OmKeyInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfo;
import org.apache.hadoop.ozone.om.helpers.OmKeyLocationInfoGroup;
+import org.apache.hadoop.ozone.recon.ReconServerConfigKeys;
import org.apache.hadoop.ozone.recon.ReconUtils;
Review Comment:
`OzoneConfiguration` and `ReconServerConfigKeys` are imported but never used
in this class, which will fail compilation (unused import). Either remove these
imports or use them to apply the CSV export max-records configuration described
in the PR.
##########
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/ContainerEndpoint.java:
##########
@@ -450,6 +459,71 @@ private Response getUnhealthyContainersFromSchema(
return Response.ok(response).build();
}
+ /**
+ * Export all unhealthy containers into a CSV file by streaming the results
directly
+ * from the database without holding them in the JVM heap.
+ *
+ * @param state The container state to filter by, or null for all.
+ * @param limit The maximum number of records to return, 0 for unlimited.
+ * @return {@link Response} containing the CSV StreamingOutput.
+ */
+ @GET
+ @Path("/unhealthy/export")
+ @Produces("text/csv")
+ public Response exportUnhealthyContainers(
+ @QueryParam("state") String state,
+ @DefaultValue("0") @QueryParam(RECON_QUERY_LIMIT) int limit) {
+
+ ContainerSchemaDefinition.UnHealthyContainerStates internalState = null;
+ if (StringUtils.isNotEmpty(state)) {
+ try {
+ internalState =
ContainerSchemaDefinition.UnHealthyContainerStates.valueOf(state);
+ } catch (IllegalArgumentException e) {
+ throw new WebApplicationException(e, Response.Status.BAD_REQUEST);
+ }
+ }
+
+ final ContainerSchemaDefinition.UnHealthyContainerStates filterState =
internalState;
+
+ StreamingOutput stream = outputStream -> {
+ try (BufferedOutputStream bos = new BufferedOutputStream(outputStream,
256 * 1024);
+ Cursor<UnhealthyContainersRecord> cursor =
+
containerHealthSchemaManager.getUnhealthyContainersCursor(filterState, limit)) {
+
+ PrintWriter writer = new PrintWriter(new OutputStreamWriter(bos,
StandardCharsets.UTF_8));
+
+ // Write CSV header
+ writer.println("container_id,container_state,in_state_since," +
+ "expected_replica_count,actual_replica_count,replica_delta");
+
+ StringBuilder sb = new StringBuilder(128);
+ while (cursor.hasNext()) {
+ UnhealthyContainersRecord rec = cursor.fetchNext();
+ sb.setLength(0);
+ sb.append(rec.getContainerId()).append(',')
+ .append(rec.getContainerState()).append(',')
+ .append(rec.getInStateSince()).append(',')
+ .append(rec.getExpectedReplicaCount()).append(',')
+ .append(rec.getActualReplicaCount()).append(',')
+ .append(rec.getReplicaDelta());
+ writer.println(sb);
+ }
+ writer.flush();
+ } catch (Exception e) {
+ LOG.error("Error streaming unhealthy containers CSV", e);
+ throw new WebApplicationException(e,
Response.Status.INTERNAL_SERVER_ERROR);
+ }
+ };
+
+ String filename = String.format("unhealthy_containers_%s_%d.csv",
+ state != null ? state.toLowerCase() : "all",
+ System.currentTimeMillis());
+
+ return Response.ok(stream)
+ .header("Content-Disposition", "attachment; filename=\"" + filename +
"\"")
+ .build();
+ }
Review Comment:
This endpoint currently allows truly unbounded exports when `limit=0` (and
also for negative limits), but the PR description calls out a hard cap via
`ozone.recon.csv.export.max.records` (default 5,000,000). Please enforce a
configured maximum here (e.g., clamp `limit` to `min(requestedLimitOrMax,
configuredMax)` and treat `0` as “use configured max”), otherwise a single
request can tie up a DB connection / request thread for an arbitrarily long
time.
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/pages/containers/containers.tsx:
##########
@@ -260,18 +288,40 @@ const Containers: React.FC<{}> = () => {
onTagClose={() => { }}
columnLength={columnOptions.length} />
</div>
- <Search
- disabled={dataToTabKeyMap[selectedTab]?.length < 1}
- searchOptions={SearchableColumnOpts}
- searchInput={searchTerm}
- searchColumn={searchColumn}
- onSearchChange={
- (e: React.ChangeEvent<HTMLInputElement>) =>
setSearchTerm(e.target.value)
- }
- onChange={(value) => {
- setSearchTerm('');
- setSearchColumn(value as 'containerID' | 'pipelineID');
- }} />
+ <div className='table-actions-section'>
+ <Search
+ disabled={dataToTabKeyMap[selectedTab]?.length < 1}
+ searchOptions={SearchableColumnOpts}
+ searchInput={searchTerm}
+ searchColumn={searchColumn}
+ onSearchChange={
+ (e: React.ChangeEvent<HTMLInputElement>) =>
setSearchTerm(e.target.value)
+ }
+ onChange={(value) => {
+ setSearchTerm('');
+ setSearchColumn(value as 'containerID' | 'pipelineID');
+ }} />
+ <Select
+ value={exportLimit}
+ onChange={(value: number) => setExportLimit(value)}
+ style={{ width: 130 }}
+ options={[
+ { value: 10000, label: '10K Records' },
+ { value: 100000, label: '100K Records' },
+ { value: 1000000, label: '1M Records' },
+ { value: 0, label: 'Complete' }
+ ]}
Review Comment:
The export limit dropdown doesn’t match the PR description (“10K up to 5
Million”) and includes a `Complete` option that sends `limit=0`, which the
backend currently interprets as unlimited. Consider adding a 5M option and
making “Complete” either omitted (so the server applies its configured max) or
explicitly set to the configured cap to avoid accidental unbounded exports.
--
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]
