You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "DenisPolo (via GitHub)" <gi...@apache.org> on 2025/12/15 06:14:57 UTC
[PR] IGNITE-27344 Enhance the heartbeat with information about maintenance… [ignite]
DenisPolo opened a new pull request, #12584:
URL: https://github.com/apache/ignite/pull/12584
… mode
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27344 Enhance the ackNodeBasicMetrics with information about … [ignite]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27344 Enhance the ackNodeBasicMetrics with information about … [ignite]
Posted by "nizhikov (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org