[crac] RFR: 8377562: [CRaC] Add better logging to RemoteJmxTest
Timofei Pushkin
tpushkin at openjdk.org
Fri Feb 20 09:54:44 UTC 2026
On Fri, 13 Feb 2026 07:45:40 GMT, Radim Vansa <rvansa at openjdk.org> wrote:
>> Makes `RemoteJmxTest` log stderr of the child process.
>
> I had just a quick peek on the changes, but while at it, could you consider to always capture the output (but `tee` it into stderr/stdout as well)?
>
> Also, I don't like the `.doRestore()` and `.doRestoreToAnalyze()` too much: what about replacing the latter with `.expect(outputAnalyzer -> ...).doRestore()`?
@rvansa
> I had just a quick peek on the changes, but while at it, could you consider to always capture the output (but tee it into stderr/stdout as well)?
Done. I was a bit hesitant because without capturing the output would still be redirected to test process'es stdout (i.e. it would be visible in the test crash log) which should be more efficient than capturing+printing. But since capturing is actually the default it is probably efficient enough for tests and we get uniform formatting this way.
> Also, I don't like the `.doRestore()` and `.doRestoreToAnalyze()` too much: what about replacing the latter with `.expect(outputAnalyzer -> ...).doRestore()`?
I considered this but I personally do not like the alternatives:
1. `.expect(outputAnalyzer -> ...).doRestore()` — firstly, `outputAnalyzer` handling is typically unique to a specific process invocation, so if there are several `doCheckpoint()`, `doRestore()`, `doPlain()` calls each will need to have its own `expect`, i.e. the lambda feels more like an argument for a specific process invocation call. Secondly, with lambdas the syntax is more awkward: I prefer `doRestoreToAnalyze().expectA().expectB()` over `.expect(o -> o.expectA().expectB()).doRestore()`, and if we need to save the analyzer for later it would be `var out = doRestoreToAnalyze()` vs `OutputAnalyzer out; b.expect(o -> { out = o; }).doRestore()`.
2. `doRestore(outputAnalyzer -> ...)` — solves the first issue above but the second one stands, plus we'd need to do `doRestore(null)` (or add `doRestore() { doRestore(null) }`) when no analysis is needed.
3. Now that we always capture output we could always return `OutputAnalyzer` but constructing one is not completely free (two `List<String>`s need to be concatenated into two long strings) and we would need to add a parameter or another method anyway to determine whether `waitForSuccess()` is needed inside `doRestore`.
I am not completely against (2) and can go that way if you prefer, but to me `doRestoreToAnalyze()` seems to provide a better syntax.
-------------
PR Comment: https://git.openjdk.org/crac/pull/295#issuecomment-3932741786
More information about the crac-dev
mailing list