nizhikov commented on PR #12584:
URL: https://github.com/apache/ignite/pull/12584#issuecomment-6037210609
Thanks for the contribution. I built the branch, ran
`MaintenanceLoggingTest` (passes) and checkstyle on `ignite-core` (clean). The
mechanics work, but I don't think the reported status is correct for the real
maintenance tasks yet. Details below.
### Blocking
1. **Status is wrong for defragmentation (the use case named in the
ticket).** `ExecuteDefragmentationAction.execute()` only starts a daemon thread
and returns `true`, so the log shows the task as `COMPLETE` within milliseconds
while defragmentation is actually running. Status is set purely around the
`execute()` call in `MaintenanceProcessor#proceedWithMaintenance`.
2. **Action results are ignored.** `ExecuteDefragmentationAction` and
`RebuildIndexAction` signal failure by returning `Boolean.FALSE`, not by
throwing. The PR only sets `FAILED` on an exception, so a failed index rebuild
is logged as `COMPLETE`.
3. **Manual actions are not tracked.** The corrupted-PDS task has no
automatic action; its actions are executed from control.sh via
`actionsForMaintenanceTask(...).get(i).execute()`, and nothing on that path
updates the state. For that task the log never shows
`ACTIVE`/`COMPLETE`/`FAILED`, which contradicts "currently executing
maintenance task and its completion status" from the ticket.
4. **`CALLED` is set on listing, not on execution.**
`actionsForMaintenanceTask` flips the status every time the action list is
retrieved. `PersistenceTask` calls it for `persistence info` too, so a
read-only control.sh query changes the reported state, and it overwrites a
previous `COMPLETE`/`FAILED`. "CALLED" also doesn't describe a task state.
Suggestion for 1–4: wrap `MaintenanceAction` in a tracking decorator for
both the automatic action and the list returned by `actionsForMaintenanceTask`,
so start, end, thrown exception and a `false` result are all observed
regardless of who triggers the action. Alternatively, drop the fine-grained
status and report only what is reliably known: active tasks and the currently
running action.
5. **Public API leak.** `MaintenanceRegistry` is public API, and the new
`tasksStatuses()` returns a preformatted string with hard-coded indentation and
`^--` prefixes. Formatting belongs in `IgniteLogInfoProviderImpl`. Expose a
collection of states, or keep it internal (there is a single implementation, so
the log provider can work with `MaintenanceProcessor` directly). Same for
`MaintenanceTaskState`: it is a logging helper but lives in the public
`org.apache.ignite.maintenance` package.
### Should fix
6. **NPE on `tasksStates.get(...)`.** `registerWorkflowCallback` does not
require the task to be active, so a callback registered for an unknown name now
NPEs in `proceedWithMaintenance` instead of just running. In-core callers are
guarded, but the API is public. Use `computeIfAbsent` or validate in
`registerWorkflowCallback`.
7. **Stale entries.** Tasks removed via `unregisterMaintenanceTask`
(including those rejected by `shouldProceedWithMaintenance` in
`prepareAndExecuteMaintenance`) stay in `tasksStates` as `REGISTERED` and keep
being logged. Remove them or show a distinct "unregistered/fixed" state.
8. **Padding baked into the enum.** `Status.val()` returns `"ACTIVE "`
etc. Alignment is a formatter concern; pad where the string is built and keep
enum values clean. The `"No maintenance tasks registered"` branch is
unreachable while `isMaintenanceMode()` is `true`.
9. **Separate log record instead of extending the metrics message.** The
ticket asks for the info in `ackNodeBasicMetrics`, but the PR emits a separate
`log.info` before the metrics block. Appending to the existing `msg` keeps one
record per period and is easier to grep.
10. **Naming / style.** `getTask`/`getStatus`/`setStatus` →
`task()`/`status()` per Ignite conventions; `mntcProc` in
`IgniteLogInfoProviderImpl` holds a `MaintenanceRegistry`, so `mntcReg`;
continuation lines use 8/12-space indents where surrounding code uses 4;
`COMPLETE` vs `COMPLETED`; the `IgniteKernal` banner has top/bottom borders but
no side borders, unlike existing banners.
### Test
11. ACTIVE state observation relies on `Thread.sleep(2000)` inside actions
racing the 1s metrics timer. A latch inside the action that blocks until the
`ACTIVE` line is observed would be deterministic.
12. Failure is simulated with JUnit `fail()` inside `execute()` and the test
then swallows `AssertionError` from `prepareAndExecuteMaintenance`. Throw a
`RuntimeException` and assert on it instead.
13. The last `actionsForMaintenanceTask(TASK_NAME + 2).get(1).execute()` has
no assertion after it and looks like leftover debugging.
14. `SimpleAction` / `SimpleMaintenanceCallback` can be static nested
classes; static `ACTION_EXECUTED` is never reset and will break if a second
test method is added.
15. No coverage for the real automatic actions (defragmentation, index
rebuild), which is exactly where points 1 and 2 bite.
### Process
Please move the JIRA to *Patch Available*, fill in the PR description, and
attach a TC.Bot visa.
--
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]
