andygrove commented on PR #5991: URL: https://github.com/apache/datafusion-comet/pull/5991#issuecomment-5743314555
Fixed in 6b504af5, and I've merged latest main. On pool coverage you're right, and the gate was the wrong lever. `createPlan` now registers unconditionally; only the counter emission stays behind `tracing_enabled`. The reasoning that convinced me is the one in your example: `tracing.enabled` is a session config while the allocation counter is process-wide, so the set of pools that contribute to allocation is not the set of pools a traced plan knows about, and there is no honest way to compare the two unless registration covers both. Your 100 MiB / 20 MiB case now reports 120 MiB and zero excess. The cost is one mutex acquisition and a map insert per `createPlan` and per `releasePlan`, which is per task, not per batch. I also folded the two reads into one. `total_reserved` takes the registry lock once and returns both the per-thread and the process-wide figure, so the pair emitted at each sample describes a single instant rather than two, and the sampling path halves its traffic through a mutex the executor needs in order to register and release pools. That is a partial answer to your performance note; the scan is still linear in live contexts, which I think is the right trade for a diagnostic that only runs when tracing is on. One thing I added that nobody asked for: the counter name now lives in `datafusion_comet_common::tracing::POOL_TOTAL_METRIC` and both the producer and the analyzer import it. Two string literals that have to stay equal, where the failure mode is a silent fall back to the warned over-counting path, seemed worth removing while I was in there. There's a new unit test for the registry-level form of your case — a pool on another thread has to appear in the process total while the per-thread counter stays local. I should be clear that it does not cover the `createPlan` gate itself, which is behind a JNI entry point a unit test cannot reach. On the Rust CI failure: that one is not this PR. `get_temp_filename` drew from 65536 random values into a single shared `target/debug/testdata` directory, and nextest runs each test in its own process with several in flight. `fs::File::create` truncates, so two tests landing on the same name leave one of them reading a Parquet file the other just emptied, and object_store reports `Requested range was invalid` because the start offset is past a now-zero-length file — which is the error you saw. Names are now process ID plus a counter. Happy to pull that out into its own PR if you'd rather keep this one to the analyzer. -- 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]
