nizhikov commented on PR #12584:
URL: https://github.com/apache/ignite/pull/12584#issuecomment-6261659829
Round 2 review (head `2a32c93`). Thanks for the quick turnaround — most of
round one is addressed: the status now lands in the metrics message,
`tasksStatuses()` is gone from the public interface, padding is out of the
enum, unregistered tasks are reflected, `CALLED`-on-listing is gone, and the
real defragmentation / index rebuild / corrupted-PDS flows have tests.
I ran checkstyle (clean), built core, and ran `MaintenanceLoggingTest` (15),
`MaintenanceDefragmentationLoggingTest` (3), `MaintenanceRegistrySimpleTest`,
`IgnitePdsDefragmentationTest`, `WalEnableDisableWithRestartsTest` and
`IndexCorruptionRebuildTest` — all green.
### Blocking
1. **Breaking change to the public `MaintenanceAction` interface.**
`state(MaintenanceTaskState)` is added as an abstract method with no default,
so every third-party `MaintenanceAction` implementation stops compiling and
existing binaries get `AbstractMethodError`. The public interface in
`org.apache.ignite.maintenance` now also imports
`org.apache.ignite.internal.maintenance.MaintenanceTaskState`, i.e. public API
depends on an internal type. The decorator approach from round one still
applies: `MaintenanceProcessor` can wrap every action it hands out (the
automatic action and the list from `actionsForMaintenanceTask`) and record
`ACTIVE` before `execute()`, `COMPLETED`/`FAILED` after, with no public API
change. Only the asynchronous defragmentation completion needs an internal
hook, which can live in `DefragmentationWorkflowCallback`. If the interface
change is kept on purpose, it needs at least a default no-op implementation and
a public (not internal) state type.
2. **Bookkeeping pushed into every action is fragile.** Each implementation
must remember `state.currAction(name()).state(ACTIVE)` plus a terminal state on
every return path. `CheckCorruptedCacheStoresCleanAction` already needs two
`COMPLETED` calls and `RebuildIndexAction` three terminal calls; an action that
forgets stays `ACTIVE` forever, and one that throws before the processor
injected the state NPEs. A single wrapper in the processor removes this class
of bugs.
3. **`MaintenanceTaskState.state` is a plain `ArrayList` mutated from
several threads.** The defragmentation thread appends, control.sh threads
append, and the metrics timer iterates it via `stream()` in `tasksStates()`.
That's a `ConcurrentModificationException` during a long defragmentation. Use
`CopyOnWriteArrayList` or synchronize; a `lastState()` accessor would also
replace the repeated `states.get(states.size() - 1)`.
4. **`testRebuildIndexActionCompletedLog` is vacuous.** The thin-client SQL
(`CREATE TABLE`, `CREATE INDEX`, 2048 inserts) never executes:
`IgniteClient.query()` returns a lazy cursor and nothing iterates it, and
core's test classpath has no SQL engine anyway. The test takes ~1.3 s;
`RebuildIndexAction` finds no index descriptor, skips, and reports `COMPLETED`.
To cover a real rebuild, the test belongs in `modules/indexing` next to
`IndexCorruptionRebuildTest`.
### Should fix
5. **`tasksStates()` returns `List<List<String>>`.** Positional string
tuples indexed 0..4 in `IgniteLogInfoProviderImpl` are hard to read and easy to
break. Return `Collection<MaintenanceTaskState>` and format from the object.
6. **`NOT_REGISTERED` state.** `registerWorkflowCallback` for a name with no
stored task fabricates a `MaintenanceTask(name, "", null)` and logs it. That
only happens on misuse (all in-core callers are guarded), and the javadoc
("registered through registerWorkflowCallback, not registered from storage") is
confusing. Reject or warn instead of inventing a state.
7. **Defragmentation `COMPLETED` after a cancel can never be recorded.**
`ExecuteDefragmentationAction` checks `state.state().contains(CANCELLED)` over
the whole history. Cancel once, start again via control.sh, let it finish →
`COMPLETED` is suppressed. Check the last state, not the history.
8. **State order for index rebuild** reads `ACTIVE->UNREGISTERED->COMPLETED`
because `unregisterMaintenanceTask()` runs before `state(COMPLETED)`. Swap them
(or record `COMPLETED` in the wrapper).
9. **Side effects in `peek`.** `actionsForMaintenanceTask` uses
`stream().peek(a -> a.state(...)).toList()`. `peek` is for debugging and may be
elided; use a plain loop.
10. **Mixed lifecycle and action states in one history.**
`REGISTERED`/`UNREGISTERED`/`NOT_REGISTERED` describe the task,
`ACTIVE`/`COMPLETED`/`FAILED`/`CANCELLED` describe an action, and action names
aren't in the history — so `ACTIVE->COMPLETED->ACTIVE->COMPLETED` for the
corrupted-PDS task doesn't say which action ran. Consider
`actions=[check=COMPLETED, clean=ACTIVE]`, or at least the last action with its
state.
11. **Style.** Single-line `if (cond) stmt;` throughout the new code
(`MaintenanceProcessor`, the actions, the log provider). Checkstyle allows it,
but surrounding code and Ignite convention put the statement on the next line.
### Test nits
12. `StopAction.execute()` in `MaintenanceLoggingTest` contains
`assertTrue(... check(2000))` inside the action itself; assertions belong in
the test method.
13. `mntcReg` is a static field reassigned per test and read from action
threads; a per-test instance field avoids cross-test leakage.
14. `testLogRegisterActiveFailedDefragmentationTask` wraps a Mockito mock of
`CachePartitionDefragmentationManager` in a real
`DefragmentationWorkflowCallback`. Works, but `IgnitePdsDefragmentationTest`
already has infrastructure for failing defragmentation that may be reusable.
### Process
PR description is still the template — please fill in what/why.
### Verdict
Not ready yet, mainly because of the public API break in `MaintenanceAction`
and the unsynchronized state list. The direction is right and the round-one
semantics issues are fixed for the covered flows; with tracking moved back into
the processor as a wrapper and the list made thread-safe, this should be close.
--
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]
