andygrove commented on code in PR #2562:
URL:
https://github.com/apache/datafusion-ballista/pull/2562#discussion_r4250229019
##########
ballista/executor/src/runtime_cache.rs:
##########
@@ -80,13 +88,21 @@ pub trait SessionRuntimeCache: Send + Sync {
/// the shared base, which installs a fresh per-task memory pool — so memory
/// isolation is unchanged.
///
+/// The cache key pairs the session id with a fingerprint of the session
+/// config's extension entries (see `config_fingerprint`). The base producer
+/// captures config state when it builds the env — the S3-aware producer
+/// clones the session's `S3Options` into the object-store registry — so a
+/// `SET` that lands between two tasks of the same session must miss the
+/// cache and build a fresh base; otherwise the new task keeps reading with
+/// the settings the session's first task carried.
+///
/// The cache is bounded by an LRU of `capacity` sessions. A capacity of `0`
Review Comment:
The capacity now counts (session, fingerprint) entries instead of sessions,
but the docs still say sessions. That's here, in the
`DefaultSessionRuntimeCache::new` docs, in the
`--session-runtime-cache-capacity` help text in `config.rs`, and in the field
doc in `executor_process.rs`. Could you update those?
A session that rotates credentials a few times will also leave stale bases
around until the LRU evicts them, and each one holds an S3 client and a footer
cache (plus the old credentials). The upper bound is unchanged so it isn't a
leak, but they can push out other sessions' warm footer caches. Dropping a
session's older entries when its fingerprint changes would avoid that, though
it would cost the "revert hits the original base" behavior. Your call on which
trade-off is better.
##########
ballista/executor/src/runtime_cache.rs:
##########
@@ -138,10 +155,52 @@ impl SessionRuntimeCache for DefaultSessionRuntimeCache {
}
}
+/// Fingerprint of the session config's extension entries.
+///
+/// A base [`RuntimeEnv`] bakes in whatever config the producer reads at build
+/// time (e.g. the `S3Options` cloned into the object-store registry), and
+/// every extension entry flows through `SET`-driven task properties, so this
+/// is the part of the config that can legitimately change mid-session.
+/// Built-in `datafusion.*` options are not included: they do not feed the
+/// base env's shared state.
+///
+/// The `ballista` extension is skipped for the same reason: none of the
+/// shipped producers read it, and the client re-sends `ballista.job.name`
+/// with every job, so keying on it would give a session that `SET`s a job
+/// name a fresh fingerprint per query — each one building a new base (and
+/// dropping the footer cache and cross-query reuse) and taking another LRU
+/// slot.
+///
+/// Extension iteration is ordered by prefix, and each extension's entries
+/// are sorted by key, so equal configs always produce equal fingerprints.
+fn config_fingerprint(config: &SessionConfig) -> u64 {
Review Comment:
Since a custom `RuntimeProducer` is a supported extension point, could we
note in the docs for `RuntimeProducer` and `SessionRuntimeCache` which config
feeds the cache key? Right now a producer that reads `datafusion.*` or
`ballista.*` options would keep the first task's values for the whole session,
and it would be good for embedders to know that. The shipped producers are
fine, so this is just about making the contract visible.
--
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]
