On Fri, 31 Jan 2025 23:23:36 GMT, Alex Menkov <amen...@openjdk.org> wrote:
> The fix implements streaming output support for attach protocol.
> See JBS issue for evaluation, summary of the changes in the 1st comment.
> Testing: tier1..4,hs-tier5-svc
Hi Alex,
Thank you for this. It's very good to have this feature. I'm still reading the
code, but I'm submitting these comments as a first round.
I didn't really understand what makes this streaming. The old protocol first
sent out the result, and then the data, has this changed?
Thanks,
Johan
src/hotspot/share/services/attachListener.cpp line 53:
> 51:
> 52: // Stream for printing attach operation results.
> 53: // Supports buffered and streaming output for commands which can produce
> lenghtly reply.
"lengthy"
src/hotspot/share/services/attachListener.cpp line 56:
> 54: //
> 55: // To support streaming output platform implementation need to implement
> AttachOperation::get_reply_writer() method
> 56: // and ctor allow_streaming argument should be set to true.
Strange mix of "need" and "should".
Can we split this into multiple sentences? Here's a suggestion, though I'm not
sure if it's 100% correct :-).
An output platform implementation supports streaming if it implements
AttachOperation::get_reply_writer().
Streaming is enabled if the allow_streaming in the constructor is set to true.
src/hotspot/share/services/attachListener.cpp line 139:
> 137: }
> 138: } else {
> 139: /* TODO: handle buffer overflow
Important `TODO` :-).
src/hotspot/share/services/diagnosticFramework.cpp line 389:
> 387: int count = 0;
> 388: while (iter.has_next()) {
> 389: if(_source == DCmd_Source_MBean && count > 0) {
Pre-existing style: Space between `if` and condition missing.
-------------
PR Review: https://git.openjdk.org/jdk/pull/23405#pullrequestreview-2592338074
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940877174
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940882015
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940886089
PR Review Comment: https://git.openjdk.org/jdk/pull/23405#discussion_r1940875712
