ArafatKhan2198 commented on code in PR #10615:
URL: https://github.com/apache/ozone/pull/10615#discussion_r3534205410
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/__tests__/capacity/Capacity.test.tsx:
##########
@@ -49,9 +49,9 @@ describe('Capacity Page', () => {
return;
}
await waitFor(() =>
- expect(ozoneCapacityCard).toHaveTextContent(/TOTAL\s*10\s*KB/i)
+ expect(ozoneCapacityCard).toHaveTextContent(/TOTAL CAPACITY\s*10\s*KB/i)
Review Comment:
The PR's main point is the **dropdown filter** (only show datanodes that
have data). That's easy to test: feed in a datanode that's missing its data,
and check it doesn't appear in the dropdown. But no such test was written.
The three test changes in the PR only *fix old, already-existing tests* they
don't check any of the new code.
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/pages/capacity/capacity.tsx:
##########
@@ -194,17 +203,23 @@ const Capacity: React.FC<object> = () => {
};
// Adjust the polling interval based on DN scan status:
- // fast (5s) while a scan is running, normal (60s) once finished.
- // Honors the auto-reload toggle: if polling is OFF, do nothing.
+ // fast (5s) while a scan is running (regardless of auto-reload toggle, since
+ // the scan is async and must be tracked to completion), normal (60s) once
+ // finished — but only if the user has auto-reload enabled.
React.useEffect(() => {
- if (!autoReload.isPolling) {
- return;
+ if (dnPendingDeletes.data.status !== "FINISHED") {
+ autoReload.startPolling(PENDING_POLL_INTERVAL);
+ } else if (autoReload.isPolling) {
+ // Scan just finished while fast-polling was active.
+ // Switch to normal interval only if the user still wants auto-reload;
+ // otherwise stop so we don't override the toggle.
+ const autoReloadEnabled = sessionStorage.getItem('autoReloadEnabled')
!== 'false';
+ if (autoReloadEnabled) {
+ autoReload.startPolling(AUTO_RELOAD_INTERVAL_DEFAULT);
+ } else {
+ autoReload.stopPolling();
+ }
}
- autoReload.startPolling(
- dnPendingDeletes.data.status === "FINISHED"
- ? AUTO_RELOAD_INTERVAL_DEFAULT
- : PENDING_POLL_INTERVAL
Review Comment:
The off-switch is broken while a scan is happening.
The new code says: "If the scan is not `FINISHED`, refresh every 5 seconds."
It doesn't check whether the user wanted refreshing at all.
So when the user flips the switch to **OFF**, the code immediately turns it
back **ON** because the scan isn't finished. It's like a light switch that
flips itself back on every time you turn it off.
Worse: if the scan **failed** or **never started**, it's *never* `FINISHED`
so the page refreshes every 5 seconds **forever**, and the user has no way to
stop it. Each refresh also hits **four** backend APIs, so it's wasteful too.
The old code checked "does the user want refreshing? If not, stop." That
worked. The new code removed that check.
---
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/__tests__/capacity/Capacity.test.tsx:
##########
@@ -63,7 +63,7 @@ describe('Capacity Page', () => {
return;
}
await waitFor(() =>
- expect(ozoneUsedSpaceCard).toHaveTextContent(/PENDING
DELETION\s*6\s*KB/i)
+ expect(ozoneUsedSpaceCard).toHaveTextContent(/PENDING
DELETION\s*8\s*KB/i)
);
});
Review Comment:
The PR's job is "fix the dropdown." The polling rewrite has nothing to do
with the dropdown, and it's the riskiest part.
The PR description just says "also fixed the polling issue found during
testing" without explaining what the issue actually was. Better to update the
description.
##########
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/pages/capacity/capacity.tsx:
##########
@@ -444,7 +459,7 @@ const Capacity: React.FC<object> = () => {
downloadUrl={DN_CSV_DOWNLOAD_URL}
onDownloadClick={() => downloadCsv(DN_CSV_DOWNLOAD_URL)}
Review Comment:
The author added this "always refresh during a scan" logic to make sure the
scan gets tracked to the end but there's *already* separate code that does
exactly that.
The CSV-download feature has its own little loop (`waitForDnFinished`) that
watches the scan until it finishes, completely independently. So the
auto-reload switch never needed to take on that job. The author solved a
problem that was already solved elsewhere and broke the toggle in the process.
--
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]
