wernerdv commented on PR #13772: URL: https://github.com/apache/ignite/pull/13772#issuecomment-6599694366
Findings that should be resolved before merge (line numbers refer to `2310066996a`): | # | Severity | Problem | How it was verified | Suggested fix | |---|---|---|---|---| | 1 | **Blocker** | A full `java.io.tmpdir` kills the JVM with exit code 1, even without a single AssertionError. The JFR repository lives there, and JFR terminates the JVM when it runs out of space. Shutdown hooks don't run, `FailureHandler` doesn't fire, and the only trace is in stderr. | Real node: 7/7 on JDK 17.0.18, 1/1 on 21.0.6. Control with `-XX:-FlightRecorder` (JDK 17): the node survived, exit code 0. | This can't be intercepted from Java, so only #2 removes the risk. Document the risk for those who enable the stream. | | 2 | **Blocker** | JFR starts on every node, including thick clients, i.e. inside the application's JVM. Ignite has no setting to turn it off. The only workaround is the deprecated `-XX:-FlightRecorder` flag, which disables JFR for the whole JVM. | `GridMetricManager.java:290`, `IgniteKernal.java:995`. Server + client in one JVM: 2 streams. The JVM itself prints the deprecation warning. | Start the stream only behind a flag in `IgniteSystemProperties`: off by default, on in TC. | | 3 | **Must fix** | The stream also starts in standalone contexts, because the call on line 290 comes before the standalone check on line 293. This affects the WAL iterator, the snapshot integrity check on a live node, `DumpReader` and CDC. Outside CDC the metric isn't exported anywhere (`StandaloneGridKernalContext.java:281`). | WAL iterator executed: `JFR Event Stream 1` appeared. The rest is from reading the code: `SnapshotPartitionsVerifyHandler.java:225`, `DumpReader.java:353`, `CdcMain.java:399`. | Move the call below the check on line 293. | | 4 | **Must fix** | A JFR startup failure breaks node start, although the metric is optional. `isAvailable()` (`GridMetricManager.java:350`) is outside the `try` and throws `NoClassDefFoundError` when the `jdk.jfr` module is missing. `catch (IllegalStateException)` (`:365`) doesn't catch `AccessControlException` either. The second scenario is narrow: a `SecurityManager` policy without `accessFlightRecorder`. | Real node, `--limit-modules` without `jdk.jfr` and a `SecurityManager` denying only that permission. In both cases start fails with the patch, and the node starts without it. | Wrap the whole method in `try`, also catch `RuntimeException` and `LinkageError`, and only log a warning. | | 5 | **Must fix** | The negative check in the test is a no-op. `waitForCondition(() -> ctr.value() == init, 2000)` (`GridMetricManagerAssertionMetricTest.java:69`) returns `true` on the first poll, while the event arrives later, up to ~1 s. The class filter is only covered indirectly by line 79. | Code reading: `GridTestUtils.java:2048-2050`. The test wasn't run. | `assertFalse(waitForCondition(() -> ctr.value() != init, 2000))` | Notes: - Performance and allocations aren't listed because no blockers were found there. JMH showed no extra allocations for ordinary exceptions and `FileChannel.write`. The main measured costs are a one-time JFR start of 0.4–0.9 s and about 48 MB of extra RSS. - The out-of-space condition was simulated on a tmpfs, not on a real partition. - Node runs used an older local build of `modules/core` with the metric classes recompiled from this commit. - The suggested fixes haven't been run. -- 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]
