fix: format NativeMemoryConsumer id in toString - #5398
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
Reviewed the complete diff from 7e0e5d2a26928b05b936c3f2a9abd375d6efc41f to 2b118654f39d14601dfd983a420bb1a82b91a424 across five independent scopes. The one-line fix addresses item 4 of #5212, and I found no correctness or compatibility issue that should block it. Approving.
Prior state and problem
NativeMemoryConsumer.toString() used NativeMemoryConsumer(id=%), so Java interpreted %) as an invalid conversion and threw UnknownFormatConversionException. Spark stringifies memory consumers in allocation, release, spill, and diagnostic logging paths; the malformed string therefore prevented those paths from reliably identifying the native consumer.
Design approach
Replace the invalid conversion with %d, using the existing immutable long plan ID. This keeps the change confined to the diagnostic representation and preserves the surrounding class, ownership, and memory-management behavior.
Correctness / compatibility analysis
%d accepts the boxed Long passed by Java varargs, including negative values and both signed-long limits. The Java and Scala signatures, JNI descriptors, allocation/release calls, and spill callback are unchanged. The string is used for diagnostics; I found no repository parser or protocol that depends on an ASCII-only representation.
A focused harness compiled the exact base and head classes against Spark 3.5.9/JDK 11, Spark 4.0.4/JDK 17, and Spark 4.1.3/JDK 17. All 61,658 assertions passed, covering the old exception, corrected output, signed-long boundaries, 1,000 deterministic random IDs, available formatting locales, and modeled partial/zero/full memory grants. This was not a full Spark/native integration run.
Key design decisions
Keep the numeric ID unchanged and repair only its format specifier. There is no new state, synchronization, dependency, or public API. The separate consumer-accounting and spilling issues tracked by #5212 remain outside this patch.
Implementation sketch
The only production change is in CometTaskMemoryManager.NativeMemoryConsumer.toString(): String.format("NativeMemoryConsumer(id=%d)", id). Existing iterator construction, the AtomicLong-backed ID source, and native acquire/release integration continue to use the same numeric values and method signatures.
Behavioral changes worth calling out
Stringifying this consumer now yields a useful diagnostic instead of an invalid-format exception. Allocation amounts, release accounting, off-heap mode, and the no-op spill callback are unchanged.
The exact-head Java lint/package matrix and Spark 4.1/JDK 17 compile-only check passed. The main CI run is still in progress; its Rust job failed on Clippy lints in spark_bit_array.rs and mersenne.rs. Both files and the relevant CI configuration are identical at the reviewed base and head, so this is not a defect introduced by this Java change.
Suggested improvements
A small regression test that directly stringifies the consumer would make this failure harder to reintroduce. Including a value beyond the 32-bit range would also document that the identifier is a long. This is a non-blocking test-coverage suggestion; no additional change is required for this fix.
2b11865 to
026c74f
Compare
Submitted in error by an automated review task. This monitor is configured for non-blocking COMMENT reviews only and must never approve.
Which issue does this PR close?
Fixing the sub issue 4 in #5212
Rationale for this change
What changes are included in this PR?
How are these changes tested?