andygrove opened a new pull request, #6436: URL: https://github.com/apache/datafusion-comet/pull/6436
Backport of #5398 to `branch-1.0`, requested in the review of #6419 ([comment](https://github.com/apache/datafusion-comet/pull/6419#discussion_r4139039797)). Cherry-picked from `1bddcff56b45876a6a5e7931ed4a6bd5dbb594b2`. It applies cleanly, and `CometTaskMemoryManager.java` ends up byte-identical to its copy on `main` right after #5398. ## Which issue does this PR close? None of its own. On `main`, #5398 addressed item 4 of the memory accounting EPIC #5212. ## Rationale for this change `NativeMemoryConsumer.toString()` on `branch-1.0` calls `String.format("NativeMemoryConsumer(id=%)", id)`. `%)` is not a valid conversion, so every call throws `UnknownFormatConversionException`. Spark's `TaskMemoryManager` passes the consumer to its DEBUG lines for each acquire and release (`Task {} acquired {} for {}`, `Task {} release {} from {}`). The `spark-core` jars for 3.4, 3.5, 4.0 and 4.1 all have these lines. log4j2 catches the exception, so nothing fails, but it logs its fallback text in place of the consumer. Anyone who sets `org.apache.spark.memory.TaskMemoryManager` to `DEBUG` to look into native memory gets lines like this for every native reservation: ```text Task 0 acquired 768.0 B for [!!!org.apache.spark.CometTaskMemoryManager$NativeMemoryConsumer@1868ed54=>java.util.UnknownFormatConversionException:Conversion = ')'!!!] ``` kazuyukitanimura spotted this in the new DEBUG test in #6419, which turns that logger on. That test passes either way, because it does not check these lines. ## What changes are included in this PR? The original one-character change: the format string uses `%d`, so `toString()` returns `NativeMemoryConsumer(id=<id>)`. Nothing is adapted for `branch-1.0`. ## How are these changes tested? #5398 added no test on `main`, and this PR adds none. The natural home for one is `CometTaskMemoryManagerSuite`, which #6419 adds to `branch-1.0`, so adding the suite here as well would make the two PRs conflict. I checked the fix locally with JDK 17 on the default profile (Spark 4.1, Scala 2.13). On top of this branch I applied #6419 and added a temporary assertion to its DEBUG test: Spark's lines must name `NativeMemoryConsumer(id=1)` and must not contain log4j2's `!!!` marker. - With this fix, the suite passes (2 tests), and the captured lines read `Task 0 acquired 768.0 B for NativeMemoryConsumer(id=1)`. - With the fix reverted, the DEBUG test fails, and the lines carry the fallback text shown above. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
