andygrove commented on PR #5613:
URL:
https://github.com/apache/datafusion-comet/pull/5613#issuecomment-5876566267
This is a light fully automated review since there are so many PRs open.
`native/core/src/execution/jni_api.rs:156-161` still explains why pool
reservations are read outside the registry lock by saying that
`CometFairMemoryPool` holds its own lock across the JNI call that acquires
memory from Spark. After this PR that is no longer true. `try_grow`, `grow` and
`shrink` all release `state` before any Spark call, `reserved()` no longer
waits on a parked acquire, and the new paragraph in `memory_management.md` says
the pool mutex is never held across a JNI call. Could that comment be updated
in this PR so the two don't contradict each other? The rule itself can stay as
a precaution. It just shouldn't cite the fair pool's lock as the reason.
The new helper in
`spark/src/test/scala/org/apache/spark/CometExecIteratorLifecycleSuite.scala:282`
captures on `classOf[CometExecIterator].getName`. The test log4j config has no
entry for that logger, so `withLogAppender` makes log4j create a non-additive
config for `org.apache.comet.CometExecIterator`, and that config outlives the
test. For the rest of that test JVM, every `CometExecIterator` log line,
including the close warning and the memory usage log, reaches no appender. That
includes any later capture on `org.apache.comet`.
`CometPluginsSuite.scala:338-341` describes this trap and listens on the
package logger for that reason. Could this test do the same with
`Seq("org.apache.comet")`? It already filters on the message text, so the
assertions would not change.
--
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]
